Use relative require paths in adblockplusui

[kzar]

Background

Previously we've used our own system for managing modules, the files bundled were specified manually in the metadata and then modules were required with no path part.

With the switch to using webpack for bundling, bundled files are resolved at build time automatically. When modules are required with no path part webpack will check the hard-coded resolve paths for suitable files, but that has several limitations / problems.

What to change

Change all require calls in adblockplusui to use relative paths. E.g.

In adblockplusui/messageResponder.js this require:

const {port} = require("messaging");

should become:

const {port} = require("../lib/messaging");

In background.js, change the implementation of require() and adapt all mock module exports to reflect the new behavior. Alternatively, replace the existing test server and use proper modules for mock logic to allow for usage of webpack directly on the test server.

[sebastian]

Replying to kzar:

In adblockplusui/messageResponder.js this require:

let ext = global.ext || require("ext_background");

should become:

let ext = global.ext || require("./ext/background");

As pointed out in the the ​review adding Webpack support to buildtools, this particular line is outdated, since ext_background only existed (as a module) in the legacy Gecko extension. We already did ​other changes to adblockplusui that break compatibility with the legacy extension. Hence require("ext_background") should rather just be removed as well, which will also avoid an unnecessary workaround in buildtools.

[jsonesen]

Hence require("ext_background") should rather just be removed as well, which will also avoid an unnecessary workaround in buildtools.

So the line should just be let ext = global.ext?

[sebastian]

As it seems this line doesn't exist anymore in adblockplusui, which likely is the reason kzar removed that part from the issue description.

[greiner]

@kzar We've been thinking about using different paths for adblockplusui so that we don't have to hardcode where repos we depend on are located since that's not determined by us but by adblockpluschrome.

For that we were thinking about something along the lines of require("webext/messaging") and require("core/filterStorage"). Since the ticket description doesn't mention which limitations/problems we encountered, I was wondering whether this approach would face the same challenges as the current one does?

Note that this is not a blocker for this change so we can finish the existing review either way.

[kzar]

It might be better to close this issue instead, and leaving the require paths alone? I'd personally prefer require("messaging") to require("webext/messaging"). The idea of this change was to reduce "magic" behaviour, instead of adding to it.

That said, my vision of how this would all work when I filed this issue hasn't really come to fruition. The new adblockplusui build system is not something I expected at the time, and I had expected adblockplusui to at some point be merged with adblockpluschrome. So, perhaps I'm not the best person to decide what makes sense now, perhaps you have a better idea.

It probably goes without saying, but whatever you choose to do please test that it works with our existing build tools first. For example, if you want to do require("webext/messaging"), make sure that's going to work in practice to build the extension.

[greiner]

Thanks for the feedback. I agree that we should reduce magic behavior as much as possible. Maybe we could think about a more Node-like dependency management so that we can require adblockpluscore the same way eslint-config-eyeo requires eslint. That way we could have the suggested require() calls and also some way of ensuring that dependencies between submodules (i.e. adblockplusui and adblockpluscore) are met while still being under control of adblockpluschrome.

If so, we could change the require() calls to this form now - in case it's not too much effort for Jon - and just switch the underlying dependency management at a later point to remove any remaining magic.

[jsonesen]

Hm, thanks for explaining that stuff guys. Maybe it is not that it's so much work to change the require behavior now but is moving from one magic behavior to another (especially one that is awaiting a further change at some point in the future) make much sense? I sort of think kzar was right in suggesting that we close the issue.

This way the final decision or spec can be compiled into one ticket and subsequently the change into one commit.

What are your thought?

[jsonesen]

I'm going to close this now since it is not something that really applies to the abpui repo