Opened 11 months ago

Last modified 7 days ago

#5761 reviewing change

Use relative require paths in adblockplusui

Reported by: kzar Assignee: jsonesen
Priority: P3 Milestone:
Module: User-Interface Keywords:
Cc: greiner, sebastian Blocked By: #5535
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29773630/

Description (last modified by greiner)

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.

Change History (10)

comment:1 in reply to: ↑ description ; follow-up: Changed 11 months ago by 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.

comment:2 Changed 11 months ago by kzar

  • Description modified (diff)

comment:3 Changed 9 months ago by greiner

  • Description modified (diff)
  • Priority changed from Unknown to P3
  • Ready set

comment:4 in reply to: ↑ 1 Changed 5 months ago by 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?

comment:5 Changed 5 months ago by 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.

comment:6 Changed 5 months ago by jsonesen

  • Owner set to jsonesen

comment:7 Changed 3 months ago by jsonesen

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:8 Changed 13 days ago by 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.

comment:9 Changed 13 days ago by 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.

comment:10 Changed 7 days ago by 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.

Note: See TracTickets for help on using tickets.