Issue 5762 - Use relative require paths

Issue 5762 - Use relative require paths

[kzar, 2017-10-18 15:12:08]

Thanks for helping out with this one.

I guess this might need to be rebased now?
Also what about lib/downloader.js?

https://codereview.adblockplus.org/29556638/diff/29556639/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29556638/diff/29556639/lib/filterListener....
lib/filterListener.js:35: const {Prefs} = require("prefs");
You missed this one, I guess it should be "../../lib/prefs"

https://codereview.adblockplus.org/29556638/diff/29556639/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29556638/diff/29556639/lib/filterStorage.j...
lib/filterStorage.js:25: const {IO} = require("io");
You also missed these.

https://codereview.adblockplus.org/29556638/diff/29556639/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29556638/diff/29556639/lib/notification.js...
lib/notification.js:24: const {Prefs} = require("prefs");
You missed this one.

https://codereview.adblockplus.org/29556638/diff/29556639/lib/notification.js...
lib/notification.js:248: applicationVersion, platform, platformVersion} =
require("info");
This one should be "../../buildtools/info" I think, but please test that works.

https://codereview.adblockplus.org/29556638/diff/29556639/lib/synchronizer.js
File lib/synchronizer.js (right):

https://codereview.adblockplus.org/29556638/diff/29556639/lib/synchronizer.js...
lib/synchronizer.js:30: const {Prefs} = require("prefs");
You missed this one.

https://codereview.adblockplus.org/29556638/diff/29556639/lib/synchronizer.js...
lib/synchronizer.js:312: const {addonVersion} = require("info");
You missed this one.

[hub, 2017-10-18 17:57:45]

I have skipped deliberately out of tree modules because we can't reference them
reliably. They are stubbed as needed by the tests.

download.js doesn't have any in tree module so it is not changed.

https://codereview.adblockplus.org/29556638/diff/29556639/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29556638/diff/29556639/lib/filterListener....
lib/filterListener.js:35: const {Prefs} = require("prefs");
On 2017/10/18 15:12:07, kzar wrote:
> You missed this one, I guess it should be "../../lib/prefs"

I don't think it is a good idea to reference out of tree modules this way. This
will cause problems with testing of adblockpluscore since prefs.js is stubbed
during the tests, and ../../lib/prefs.js here leads nowhere since
adblockpluscore is checked out separately.

https://codereview.adblockplus.org/29556638/diff/29556639/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29556638/diff/29556639/lib/filterStorage.j...
lib/filterStorage.js:25: const {IO} = require("io");
On 2017/10/18 15:12:07, kzar wrote:
> You also missed these.

same. out of tree module.

https://codereview.adblockplus.org/29556638/diff/29556639/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29556638/diff/29556639/lib/notification.js...
lib/notification.js:24: const {Prefs} = require("prefs");
On 2017/10/18 15:12:07, kzar wrote:
> You missed this one.

same. out of tree module.

https://codereview.adblockplus.org/29556638/diff/29556639/lib/notification.js...
lib/notification.js:248: applicationVersion, platform, platformVersion} =
require("info");
On 2017/10/18 15:12:07, kzar wrote:
> This one should be "../../buildtools/info" I think, but please test that
works.

out of tree module.

https://codereview.adblockplus.org/29556638/diff/29556639/lib/synchronizer.js
File lib/synchronizer.js (right):

https://codereview.adblockplus.org/29556638/diff/29556639/lib/synchronizer.js...
lib/synchronizer.js:30: const {Prefs} = require("prefs");
On 2017/10/18 15:12:08, kzar wrote:
> You missed this one.

out of tree module.

https://codereview.adblockplus.org/29556638/diff/29556639/lib/synchronizer.js...
lib/synchronizer.js:312: const {addonVersion} = require("info");
On 2017/10/18 15:12:08, kzar wrote:
> You missed this one.

out of tree module

[hub, 2017-10-18 18:04:41]

rebasing led to no changes it seems.

[kzar, 2017-10-20 13:51:30]

On 2017/10/18 17:57:45, hub wrote:
> I have skipped deliberately out of tree modules because we can't reference
them
> reliably. They are stubbed as needed by the tests.

In my opinion we should use relative paths for these too. That's kind of the
point, being able to explicitly require a module in one repository from another.
When we require "prefs" here we do mean ../../lib/prefs.js. Whoever is using the
core code must provide that module for it to work. Once we do that everywhere
we'll no longer have to give modules names like "coreUtils" in order to avoid
the name of a module in one repository clashing with a module from another.

This is not something I discussed with Sebastian or Wladimir yet though, so I'd
be interested to get their opinion.

As for stubbing those modules for the unit tests, I don't think it's a problem.
At worst we could add resolve aliases[1] in our webpack configuration in
test_runner.js for them.

[1] - https://webpack.js.org/configuration/resolve/#resolve-alias

[sergei, 2018-01-03 12:57:18]

On 2017/10/20 13:51:30, kzar wrote:
> On 2017/10/18 17:57:45, hub wrote:
> > I have skipped deliberately out of tree modules because we can't reference
> them
> > reliably. They are stubbed as needed by the tests.
> 
> In my opinion we should use relative paths for these too. That's kind of the
> point, being able to explicitly require a module in one repository from
another.
> When we require "prefs" here we do mean ../../lib/prefs.js. Whoever is using
the
> core code must provide that module for it to work. Once we do that everywhere
> we'll no longer have to give modules names like "coreUtils" in order to avoid
> the name of a module in one repository clashing with a module from another.
> 
> This is not something I discussed with Sebastian or Wladimir yet though, so
I'd
> be interested to get their opinion.
> 
> As for stubbing those modules for the unit tests, I don't think it's a
problem.
> At worst we could add resolve aliases[1] in our webpack configuration in
> test_runner.js for them.
> 
> [1] - https://webpack.js.org/configuration/resolve/#resolve-alias

IMO such paths like ../../lib/prefs.js and ../../buildtools/info are pretty
specific for one particular outer project, in particular they make no sense in
libadblockplus, and a library should not be aware about any outer project. I
don't mind to change the paths to prefs.js and to "info" but the new path should
reflect that it's a part of adblockplus library and possibly that these modules
should be provided externally. Therefore, having an alias actually looks good
here, what about require("abp-modules/prefs.js") and
require("abp-modules/info")? abp-modules refers to adblockplus modules.

[hub, 2018-01-03 20:13:42]

I followed sergei suggestion to use "abp-modules" as path prefix.

We'll need to change the add-on to work with these changes.

[kzar, 2018-01-04 13:10:20]

On 2018/01/03 12:57:18, sergei wrote:

> IMO such paths like ../../lib/prefs.js and ../../buildtools/info are pretty
> specific for one particular outer project, in particular they make no sense in
> libadblockplus, and a library should not be aware about any outer project.

I'm absolutely against adding this special "abp-modules" path prefix. The whole
purpose of this change and the related changes was to use more standard syntax
for our modules! We'd be better off leaving those requires as they were.

adblockpluscore depends on code from the parent repository, there's no two ways
about it. Hiding that as it is by having webpack resolve the modules
automatically, or with this special "abp-modules" prefix doesn't change that
fact.

[sergei, 2018-01-04 13:17:07]

On 2018/01/04 13:10:20, kzar wrote:
> On 2018/01/03 12:57:18, sergei wrote:
> 
> > IMO such paths like ../../lib/prefs.js and ../../buildtools/info are pretty
> > specific for one particular outer project, in particular they make no sense
in
> > libadblockplus, and a library should not be aware about any outer project.
> 
> I'm absolutely against adding this special "abp-modules" path prefix. The
whole
> purpose of this change and the related changes was to use more standard syntax
> for our modules! We'd be better off leaving those requires as they were.
> 
> adblockpluscore depends on code from the parent repository, there's no two
ways
> about it. Hiding that as it is by having webpack resolve the modules
> automatically, or with this special "abp-modules" prefix doesn't change that
> fact.

Then let's just leave those requires as they were.

[kzar, 2018-01-04 14:22:14]

On 2018/01/04 13:17:07, sergei wrote:

> IMO such paths like ../../lib/prefs.js and ../../buildtools/info are pretty
> specific for one particular outer project, in particular they make no sense
> in libadblockplus, and a library should not be aware about any outer project.

Well I've just taken a look at libadblockplus and there is a
libadblockplus/lib/prefs.js file, so require("../../lib/prefs") seems OK there.

As for info.js it lives in libadblockplus/lib/info.js instead of
libadblockplus/buildtools/info.js so I guess you're right there. We need to
alter the magic in buildtools/webpack_runner.js for the adblockpluschrome build,
so that it considers the file to live in adblockpluschrome/lib/info.js instead
of adblockpluschrome/buildtools/info.js. I've opened #6230 for that and will
look at fixing it now, in the mean time Hubert you can assume that the info
module lives in ../../lib/info.js.

[sergei, 2018-01-04 15:49:10]

On 2018/01/04 14:22:14, kzar wrote:
> On 2018/01/04 13:17:07, sergei wrote:
> 
> > IMO such paths like ../../lib/prefs.js and ../../buildtools/info are pretty
> > specific for one particular outer project, in particular they make no sense
> > in libadblockplus, and a library should not be aware about any outer
project.
> 
> Well I've just taken a look at libadblockplus and there is a
> libadblockplus/lib/prefs.js file, so require("../../lib/prefs") seems OK
there.
> 
> As for info.js it lives in libadblockplus/lib/info.js instead of
> libadblockplus/buildtools/info.js so I guess you're right there. We need to
> alter the magic in buildtools/webpack_runner.js for the adblockpluschrome
build,
> so that it considers the file to live in adblockpluschrome/lib/info.js instead
> of adblockpluschrome/buildtools/info.js. I've opened #6230 for that and will
> look at fixing it now, in the mean time Hubert you can assume that the info
> module lives in ../../lib/info.js.

Actually the implementation in libadblockplus is pretty hacky and it works only
with basenames of files, so ../../lib is not used anyway. But it's not a crucial
factor.

In general I am still not convinced to have relative paths to something in the
parent project because I think that a library should not require, if it's
possible, a special directory structure in a parent project. It can be very
difficult to maintain and work with such library. In particular it can clash
with already existing "lib" directory, or it's a common practice to keep such
libraries in some "third_party", "vendors" or even in a directory out of a
project tree, what will require to have some intermediate third_party/lib which
can be undesired, or how could one create the npm module from adblockpluscore?

A common approach to obtain some externally prepared dependencies, beside a
direct injection in a code, is to use search paths, for example depending on a
technology via various environment variables like PATH, NODE_PATH, LD_PRELOAD,
CLASSPATH (Java), PYTHONPATH, etc, or gcc has -I and -L options for search paths
for headers and libraries, webpack has the resolver library to keep it flexible.

IMO, it should be similar here. Having some prefix like "abp-modules/" would
probably prevent from potential name clashing but it's a next step.

[hub, 2018-01-04 17:05:07]

On 2018/01/04 15:49:10, sergei wrote:

> In general I am still not convinced to have relative paths to something in the
> parent project because I think that a library should not require, if it's
> possible, a special directory structure in a parent project. It can be very
> difficult to maintain and work with such library. In particular it can clash
> with already existing "lib" directory, or it's a common practice to keep such
> libraries in some "third_party", "vendors" or even in a directory out of a
> project tree, what will require to have some intermediate third_party/lib
which
> can be undesired, or how could one create the npm module from adblockpluscore?
> 
> A common approach to obtain some externally prepared dependencies, beside a
> direct injection in a code, is to use search paths, for example depending on a
> technology via various environment variables like PATH, NODE_PATH, LD_PRELOAD,
> CLASSPATH (Java), PYTHONPATH, etc, or gcc has -I and -L options for search
paths
> for headers and libraries, webpack has the resolver library to keep it
flexible.
> 
> IMO, it should be similar here. Having some prefix like "abp-modules/" would
> probably prevent from potential name clashing but it's a next step.


My take is similar to sergei, which was my original one: 

JS modules that are not in the component (ie not in adblockpluscore) should be
loaded without a relative path and the module loader should set these path
properly. This applies to the tests (that perform a rewrite instead because
nodunit is a bit incomplete), or to the add-on that uses webpack and therefor
supplies the path when transforming the source.

"abp-modules/" provide a nice namespacing so that we can ensure that the right
modules are loaded instead of pulling from some random npm package. This is what
this patch uses, and deal with the rewriting in the test bootstrap.

Assuming as specific layout for the "client" of adblockpluscore (ie the
component that uses it) is a very leaky abstraction.

[kzar, 2018-01-04 18:06:14]

On 2018/01/04 15:49:10, sergei wrote:

> Actually the implementation in libadblockplus is pretty hacky and it works
only
> with basenames of files, so ../../lib is not used anyway. But it's not a
crucial
> factor.

Well if the implementation in libadblockplus is hacky that should also be fixed
right?

On 2018/01/04 17:05:07, hub wrote:

> "abp-modules/" provide a nice namespacing so that we can ensure that the right
> modules are loaded instead of pulling from some random npm package. This is
what
> this patch uses, and deal with the rewriting in the test bootstrap.

So long as the modules are prefixed with "abp-modules/" this does NOT LGTM.

> Assuming as specific layout for the "client" of adblockpluscore (ie the
> component that uses it) is a very leaky abstraction.

To avoid an impasse I'll accept leaving the paths non-relative (unchanged) for
the ones residing in the parent repository.

[hub, 2018-01-04 20:54:51]

> > Assuming as specific layout for the "client" of adblockpluscore (ie the
> > component that uses it) is a very leaky abstraction.
> 
> To avoid an impasse I'll accept leaving the paths non-relative (unchanged) for
> the ones residing in the parent repository.

I am OK with that. Will update the patch.

[hub, 2018-01-04 21:01:52]

patch updated.

[sergei, 2018-01-05 08:21:42]

LGTM

[kzar, 2018-01-05 14:20:40]

LGTM