Issue 6625 - Expose webpack's resolve.alias to the packagers

Issue 6625 - Expose webpack's resolve.alias to the packagers

We might want to adjust adblockpluschrome's README afterwards, to name this exposure and it's limits (i.e. only requires from the configured resolve_paths can be aliased)

[tlucas, 2018-04-26 08:44:06]

Patch Set 1

 * Expose resolve.alias fully to create_bundles()

[Sebastian Noack, 2018-04-26 11:30:39]

https://codereview.adblockplus.org/29762564/diff/29762565/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29762564/diff/29762565/packagerChrome.py#n...
packagerChrome.py:17: import ConfigParser
Nit: ConfigParser is a corelib module. So it goes together with the other
corelib modules without a blank line in between.

https://codereview.adblockplus.org/29762564/diff/29762565/packagerChrome.py#n...
packagerChrome.py:173: {k: v for k, v in
params['metadata'].items('module_alias')},
Nit: The dict comprehension here is redundant.

[tlucas, 2018-04-26 11:40:58]

Patch Set 2
 * Addressed Sebastian's comments

https://codereview.adblockplus.org/29762564/diff/29762565/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29762564/diff/29762565/packagerChrome.py#n...
packagerChrome.py:17: import ConfigParser
On 2018/04/26 11:30:39, Sebastian Noack wrote:
> Nit: ConfigParser is a corelib module. So it goes together with the other
> corelib modules without a blank line in between.

Done.

https://codereview.adblockplus.org/29762564/diff/29762565/packagerChrome.py#n...
packagerChrome.py:173: {k: v for k, v in
params['metadata'].items('module_alias')},
On 2018/04/26 11:30:39, Sebastian Noack wrote:
> Nit: The dict comprehension here is redundant.

Done.

[Sebastian Noack, 2018-04-26 15:10:27]

LGTM. But I want kzar to review this change as well, since he is a little bit
more familiar with the webpack stuff.

[tlucas, 2018-04-27 09:09:09]

On 2018/04/26 15:10:27, Sebastian Noack wrote:
> LGTM. But I want kzar to review this change as well, since he is a little bit
> more familiar with the webpack stuff.

I agree - Dave, would you be so kind? :)

[kzar, 2018-04-30 11:05:08]

I would have preferred it if we could have figured out a way to work with the
mapping section instead, but I think in practice that would have been work and
more of a mess. Just look at the hacks required for the info module for example.

So that said I think you're approach looks pretty much OK, I have just one bit
of feedback regarding requiring and aliasing modules using relative paths.

https://codereview.adblockplus.org/29762564/diff/29762578/tests/metadata.edge
File tests/metadata.edge (right):

https://codereview.adblockplus.org/29762564/diff/29762578/tests/metadata.edge...
tests/metadata.edge:43: mogo = edge
I think it's better specify the relative path for the module you're aliasing to.
For example "mongo = ./path/to/edge".

I'd also like to see an example of an alias for a relatively required module,
e.g. "./lib/mongo = ./some/path/lib/edge" - would that work? 

But this makes me think, I guess we should be careful with these paths. For
example if the path for an alias is given as ./lib/example.js in
adblockpluschrome/metadata.whatever, will that point to
adblockpluschrome/lib/example.js like it should, or to
adblockpluschrome/buildtools/lib/example.js since the scripts inside buildtools
are dealing with the alias paths?

[tlucas, 2018-04-30 13:01:06]

Hey Dave, thanks for your comments!

Please find the discussion below, cheers!

https://codereview.adblockplus.org/29762564/diff/29762578/tests/metadata.edge
File tests/metadata.edge (right):

https://codereview.adblockplus.org/29762564/diff/29762578/tests/metadata.edge...
tests/metadata.edge:43: mogo = edge
On 2018/04/30 11:05:08, kzar wrote:
> I think it's better specify the relative path for the module you're aliasing
to.
> For example "mongo = ./path/to/edge".

I'm sorry, could you clarify what you mean? "mogo = edge" already is a relative
alias, resolving 'require("mogo")' from "./lib/mogo.js" to "./lib/edge.js"

> 
> I'd also like to see an example of an alias for a relatively required module,
> e.g. "./lib/mongo = ./some/path/lib/edge" - would that work? 
> 
> But this makes me think, I guess we should be careful with these paths. For
> example if the path for an alias is given as ./lib/example.js in
> adblockpluschrome/metadata.whatever, will that point to
> adblockpluschrome/lib/example.js like it should, or to
> adblockpluschrome/buildtools/lib/example.js since the scripts inside
buildtools
> are dealing with the alias paths?

Taken from the resolve.alias documentation: "If the value is a relative path it
will be relative to the file containing the require."
So resolving doesn't depend on the metadata.*'s location or the location of
webpack_runner.js (and it should be no concern for us)

However, what i provided here as test data is meant as mere check if webpack's
aliasing actually is exposed. Imho it's not up to us to re-test something
implemented by webpack, but if you think it would make sense, i'd add a few more
checks.

[kzar, 2018-04-30 13:53:13]

https://codereview.adblockplus.org/29762564/diff/29762578/tests/metadata.edge
File tests/metadata.edge (right):

https://codereview.adblockplus.org/29762564/diff/29762578/tests/metadata.edge...
tests/metadata.edge:43: mogo = edge
On 2018/04/30 13:01:06, tlucas wrote:
> Taken from the resolve.alias documentation: "If the value is a relative path
it
> will be relative to the file containing the require."
> So resolving doesn't depend on the metadata.*'s location or the location of
> webpack_runner.js (and it should be no concern for us)

Right fair enough, 

> However, what i provided here as test data is meant as mere check if webpack's
> aliasing actually is exposed. Imho it's not up to us to re-test something
> implemented by webpack, but if you think it would make sense, i'd add a few
more
> checks.

Yea, I think you're right. We're not testing WebPack here, but instead the
scripts generating the WebPack configuration.

> On 2018/04/30 11:05:08, kzar wrote:
> > I think it's better specify the relative path for the module you're aliasing
> to.
> > For example "mongo = ./path/to/edge".
> 
> I'm sorry, could you clarify what you mean? "mogo = edge" already is a
relative
> alias, resolving 'require("mogo")' from "./lib/mogo.js" to "./lib/edge.js"

Well if I type `require("./lib/info")` Webpack will include the file at that
location relative to the current file (with the .js extension). No surprises
there. But if I type `require("info");` WebPack will attempt to resolve the info
module for me, checking the resolve paths we defined and IIRC the current path.
If there are multiple modules of that name in different (resolved) locations who
knows which will be included. This ends up in a mess, with us naming modules
stuff like "coreUtils" instead of "utils" since we can't handle two modules
being named "utils", even if they're in different repositories.

So take your example `require("mogo")` - that's not what I mean about a relative
require, that would be something like `require("./lib/mogo")`.

Anyway like you point out we're testing the script generates the right
configuration here, not WebPack itself. So no matter about changing these tests,
but I'm more worried about how this will work in practice.

For example if the code has a relative require like `require(./lib/whatever)`
will we be able to alias it to point to a different file? If not I guess we'd
have to modify those requires to not specify the path and then hope there
doesn't happen to be another module of the same name somewhere else.

[kzar, 2018-04-30 14:00:14]

https://codereview.adblockplus.org/29762564/diff/29762578/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29762564/diff/29762578/packagerChrome.py#n...
packagerChrome.py:171: aliases.update(params['metadata'].items('module_alias'))
I guess maybe we should make the vales an absolute path relative to the metadata
file that the entry came from?

[kzar, 2018-04-30 17:37:31]

Tristan and I discussed this for a while in IRC today, the summary seems to be
that neither of us are 100% sure how aliases work with regards to relative and
absolute paths. Tristan is going to do some experimenting to make sure his
implementation as it stands can be used in practice for what we want (replacing
one module with another depending on the target build), but assuming that it can
then this LGTM.

[tlucas, 2018-04-30 17:58:16]

On 2018/04/30 17:37:31, kzar wrote:
> Tristan and I discussed this for a while in IRC today, the summary seems to be
> that neither of us are 100% sure how aliases work with regards to relative and
> absolute paths. Tristan is going to do some experimenting to make sure his
> implementation as it stands can be used in practice for what we want
(replacing
> one module with another depending on the target build), but assuming that it
can
> then this LGTM.

I could confirm that this change does, what it's supposed to do (with the
example of #6621)
I also added some integration notes to #6625, in order to mention this machinery
in the README's after updating to the new revision.