Opened 22 months ago

Closed 8 weeks ago

#6230 closed change (rejected)

Alter resolve path of info module generated by buildtools

Reported by: kzar Assignee:
Priority: Unknown Milestone:
Module: Automation Keywords: goodfirstbugs
Cc: tlucas, sebastian, hfiguiere, sergz, erikvold, geo Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29656586/

Description (last modified by kzar)

Background

Recently we switched to using Webpack for our module system when building adblockpluschrome. We're now able to adjust the require paths to be relative and that includes the requires in adblockpluscore (#5762).

With how we've configured Webpack currently the special info module which is generated at build time is considered to be located in adblockpluschrome/buildtools/info.js when building the extension. This causes us some problems (see this discussion) since libadblockplus stores the file in libadblockplus/lib/info.js. For require("../../lib/info") to work for both libadblockplus and adblockpluchrome we need to adjust our Webpack configuration.

What to change

Adjust webpack_runner.js so that the info alias points to the parent directory, e.g.

info$: path.join(__dirname, "info.js"),

becomes

info$: path.join(extension_path, "lib", "info.js"),

Remove the stub buildtools/info.js file.

Integration notes

  • Create an stub file in adblockpluschrome/lib/info.js.
  • Ensure any relative requires for the info module are updated with the correct path.

Change History (7)

comment:1 Changed 22 months ago by kzar

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

comment:2 Changed 22 months ago by kzar

  • Description modified (diff)

comment:3 Changed 22 months ago by kzar

  • Blocking 5762 removed

Since there was disagreement about calling require("../../lib/info") from adblockpluscore this change is no longer blocking #5762. Nevertheless it seems to me a sensible change to make so I'll leave this issue open.

comment:4 Changed 19 months ago by kzar

  • Keywords goodfirstbugs added
  • Status changed from reviewing to reopened

comment:5 Changed 19 months ago by kzar

  • Owner kzar deleted

comment:6 Changed 11 months ago by erikvold

  • Cc erikvold added

comment:7 Changed 8 weeks ago by sebastian

  • Cc geo added
  • Resolution set to rejected
  • Status changed from reopened to closed

Maybe this is something that we can address in the new build suite, Geo is working on. But as our legacy buildtools is on the way out, we won't address it there for the time being.

Note: See TracTickets for help on using tickets.