Issue 29773630: Issue 5761 - Changes require paths to relative (Closed)
|
Created:
May 8, 2018, 1:03 a.m. by Jon Sonesen Modified:
Jan. 29, 2019, 3:56 p.m. CC:
kzar Base URL:
https://hg.adblockplus.org/adblockplusui/ Visibility:
Public. |
DescriptionIssue 5761 - Change module paths to be relative
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address PS1 comment #
Total comments: 4
Patch Set 3 : Address PS2 comment #
MessagesTotal messages: 13
https://codereview.adblockplus.org/29773630/diff/29773631/background.js File background.js (right): https://codereview.adblockplus.org/29773630/diff/29773631/background.js#newco... background.js:93: return modules[module.substr(module.lastIndexOf("/") + 1)]; this is a bit hacky and doesn't help if multiple modules share the same name, but not sure how much of a problem that is.
https://codereview.adblockplus.org/29773630/diff/29773631/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29773630/diff/29773631/messageResponder.js... messageResponder.js:40: const info = require("../buildtools/info"); It turns out using a relative path for the info module is a bad idea, it caused problems for the AdBlock guys (see #6675). Please could you leave this one as `const info = require("info");`?
On 2018/05/17 13:53:28, kzar wrote: > https://codereview.adblockplus.org/29773630/diff/29773631/messageResponder.js > File messageResponder.js (right): > > https://codereview.adblockplus.org/29773630/diff/29773631/messageResponder.js... > messageResponder.js:40: const info = require("../buildtools/info"); > It turns out using a relative path for the info module is a bad idea, it caused > problems for the AdBlock guys (see #6675). Please could you leave this one as > `const info = require("info");`? Ack
On 2018/05/18 17:18:12, Jon Sonesen wrote: > Ack Thanks
a minor note on the name resolution just because I've read kzar note. If that's not a concern, then this looks OK to me. https://codereview.adblockplus.org/29773630/diff/29785577/background.js File background.js (right): https://codereview.adblockplus.org/29773630/diff/29785577/background.js#newco... background.js:93: return modules[module.substr(module.lastIndexOf("/") + 1)]; this is probably already fine but assumes there is a `/` in the path. What always works and produces same result instead is `module.split('/').pop()`, so that even if there's no slash in the path, you'll have a module to load (i.e. the "info" one, whenever it gets involved in here). Feel free to ignore this comment if there won't ever be a case without a slash.
https://codereview.adblockplus.org/29773630/diff/29785577/background.js File background.js (right): https://codereview.adblockplus.org/29773630/diff/29785577/background.js#newco... background.js:93: return modules[module.substr(module.lastIndexOf("/") + 1)]; On 2018/07/02 08:14:26, a.giammarchi wrote: > this is probably already fine but assumes there is a `/` in the path. > > What always works and produces same result instead is `module.split('/').pop()`, > so that even if there's no slash in the path, you'll have a module to load (i.e. > the "info" one, whenever it gets involved in here). > > Feel free to ignore this comment if there won't ever be a case without a slash. Yeah, previously there were never `/` characters but now i think they will always appear (perhaps the info module is the exception here) Thomas, Dave what do you think?
https://codereview.adblockplus.org/29773630/diff/29785577/background.js File background.js (right): https://codereview.adblockplus.org/29773630/diff/29785577/background.js#newco... background.js:93: return modules[module.substr(module.lastIndexOf("/") + 1)]; On 2018/07/09 19:23:49, Jon Sonesen wrote: > Yeah, previously there were never `/` characters but now i think they will > always appear (perhaps the info module is the exception here) Thomas, Dave what > do you think? I'd say let's go with whichever requires making fewer assumptions. So in this case that would be `module.split("/").pop()`. https://codereview.adblockplus.org/29773630/diff/29785577/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29773630/diff/29785577/messageResponder.js... messageResponder.js:27: const {FilterStorage} = require("../adblockpluscore/lib/filterStorage"); It'd be great if we could avoid hardcoding the location of any of the modules we rely on so that we are not dependent on the file structure of the super repo. Because while there is technically a bidirectional dependency between adblockpluschrome and adblockplusui, the former is in control of when and how to update the dependency. So in an ideal scenario we could write requires as `require("webext/messaging")` and `require("core/filterStorage")` so that we can define the paths in a configuration file. What do you think about that approach?
On 2018/07/26 10:08:00, Thomas Greiner wrote: > https://codereview.adblockplus.org/29773630/diff/29785577/background.js > File background.js (right): > > https://codereview.adblockplus.org/29773630/diff/29785577/background.js#newco... > background.js:93: return modules[module.substr(module.lastIndexOf("/") + 1)]; > On 2018/07/09 19:23:49, Jon Sonesen wrote: > > Yeah, previously there were never `/` characters but now i think they will > > always appear (perhaps the info module is the exception here) Thomas, Dave > what > > do you think? > > I'd say let's go with whichever requires making fewer assumptions. So in this > case that would be `module.split("/").pop()`. > > https://codereview.adblockplus.org/29773630/diff/29785577/messageResponder.js > File messageResponder.js (right): > > https://codereview.adblockplus.org/29773630/diff/29785577/messageResponder.js... > messageResponder.js:27: const {FilterStorage} = > require("../adblockpluscore/lib/filterStorage"); > It'd be great if we could avoid hardcoding the location of any of the modules we > rely on so that we are not dependent on the file structure of the super repo. > Because while there is technically a bidirectional dependency between > adblockpluschrome and adblockplusui, the former is in control of when and how to > update the dependency. > > So in an ideal scenario we could write requires as `require("webext/messaging")` > and `require("core/filterStorage")` so that we can define the paths in a > configuration file. What do you think about that approach? FWIW I'd love to have that in too: +1
On 2018/07/26 10:24:41, a.giammarchi wrote: > On 2018/07/26 10:08:00, Thomas Greiner wrote: > > https://codereview.adblockplus.org/29773630/diff/29785577/background.js > > File background.js (right): > > > > > https://codereview.adblockplus.org/29773630/diff/29785577/background.js#newco... > > background.js:93: return modules[module.substr(module.lastIndexOf("/") + 1)]; > > On 2018/07/09 19:23:49, Jon Sonesen wrote: > > > Yeah, previously there were never `/` characters but now i think they will > > > always appear (perhaps the info module is the exception here) Thomas, Dave > > what > > > do you think? > > > > I'd say let's go with whichever requires making fewer assumptions. So in this > > case that would be `module.split("/").pop()`. > > > > https://codereview.adblockplus.org/29773630/diff/29785577/messageResponder.js > > File messageResponder.js (right): > > > > > https://codereview.adblockplus.org/29773630/diff/29785577/messageResponder.js... > > messageResponder.js:27: const {FilterStorage} = > > require("../adblockpluscore/lib/filterStorage"); > > It'd be great if we could avoid hardcoding the location of any of the modules > we > > rely on so that we are not dependent on the file structure of the super repo. > > Because while there is technically a bidirectional dependency between > > adblockpluschrome and adblockplusui, the former is in control of when and how > to > > update the dependency. > > > > So in an ideal scenario we could write requires as > `require("webext/messaging")` > > and `require("core/filterStorage")` so that we can define the paths in a > > configuration file. What do you think about that approach? > > FWIW I'd love to have that in too: +1 Ok, I think doing this would not be using relative paths anymore and defeat the purpose of using webpack?
On 2018/07/27 01:16:52, Jon Sonesen wrote: > > I think doing this would not be using relative paths anymore and defeat the > purpose of using webpack? it's not black or white, it's about internal files a folder owns VS "third parts" and you can also configure WebPack to have those exceptions for "third parts". Anyway, my "I'd love to have that" didn't mean to block this or anything, was a comment on what Thomas said. To me, this is still OK to go as previously written.
I agree that there's probably no need to block this change because of that. However, in case it turns out that it's not much extra work, it'd be a nice change to include.
On 2018/07/27 09:30:35, Thomas Greiner wrote: > I agree that there's probably no need to block this change because of that. > However, in case it turns out that it's not much extra work, it'd be a nice > change to include. After looking back at this the original ticket assumed that everyone used the same webpack etc when in practice tis just isn't true so I think it is better to close this 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
