Opened 8 months ago

Last modified 9 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 (7)

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

  • Description modified (diff)

comment:3 Changed 7 months ago by greiner

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

comment:4 in reply to: ↑ 1 Changed 3 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 3 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 8 weeks ago by jsonesen

  • Owner set to jsonesen

comment:7 Changed 9 days ago by jsonesen

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing
Note: See TracTickets for help on using tickets.