Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#5535 closed change (fixed)

Begin using webpack for script bundling in the Chrome packager

Reported by: kzar Assignee: kzar
Priority: P2 Milestone:
Module: Automation Keywords:
Cc: tlucas, sebastian, trev, hfiguiere Blocked By: #5559, #5596, #5856
Blocking: #5080, #5383, #5383, #5760, #5761, #5762 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29549786/
https://codereview.adblockplus.org/29573821/
https://codereview.adblockplus.org/29574582/

Description (last modified by kzar)

Background

The adblockpluschrome code relies on a (CommonJS-ish) module system to allow for a separation of concerns. For example the popup blocking code in adblockpluschrome/lib/popupBlocker.js can make use of shared URL processing code in adblockpluschrome/lib/url.js by calling something like let stringifyURL = require("url").stringifyURL". That takes the stringifyURL function from the url module and makes it available for use.

Since browsers don't have built in support for modules like Node.js does, we've implemented the require function and related logic ourselves. Scripts listed in the [convert_js] section of the metadata files with the [module] = true option specified are wrapped in our module logic. If the [autoload] option is given the listed modules will be required automatically, if the [injectinfomodule] option is given the info module will be generated and included too. convertJS in buildtools/packagerChrome.py does that work, making use of the buildtools/templates/modules.js.tmpl template.

Currently the logic in packagerChrome.py doesn't work properly when it comes to module paths. As it stands we take the file's name without the extension part. Then we take the last directory part in the file's path, if it's "lib" we drop it and otherwise we prefix it with an underscore. So lib/example.js becomes example where as lib/foo/bar/example.js becomes bar_example.

We now need proper path support for require. We've decided to using switch to using webpack to achieve that, rather than continue maintaining our own system. Webpack is a widely used tool that can do exactly what we need here.

What to change

  • Add the "webpack": "3.5.5" dependency to buildtools/package.json.
  • Include the changes to buildtools/package-lock.json.
  • Adjust convertJS in buildtools/packagerChrome.py to call out to webpack to do the bundling of JavaScript files. (The path of the dynamically included info module should be considered to be lib/info.js.)
  • Make sure that webpack generates a sourcemap file and include it in the build, to make debugging easier.
  • Remove the buildtools/templates/modules.js.tmpl file entirely.

Change the [convert_js] metadata API as follows:

  • Rename it to [bundles]
  • Remove the modules option, effectively that should always be true.
  • Remove the [autoload] option, any files listed will be considered auto-loaded modules (entry points) and webpack will resolve any others automatically.
  • Remove the [injectinfomodule] option - webpack will automatically include the info module if it's required.

Change the logic so that any mapped files which are also included in a bundle have their mapping removed. Otherwise they will be included twice, once in the bundle and again from the mapping.

Integration notes

  • Take care to fix the path of the content/elemHideEmulation require in include.preload.js.
  • Convert all content scripts into proper modules which require from one another.
  • Remove all the /* global */ comments where possible.
  • Adjust the metadata files to use the new API.

Change History (47)

comment:1 Changed 2 years ago by kzar

Mind checking this Sebastian and Wladimir? I'm curious if you agree about how this should all work.

This will require both Python and JavaScript changes Tristan, so it might be an issue we could work together on. Up to you anyway, how's your JavaScript?

comment:2 Changed 2 years ago by kzar

  • Description modified (diff)

comment:3 Changed 2 years ago by tlucas

My JS can be broken down to "i patched some things here and there" - but in the end i should be able to read myself into stuff.

However, i'm happy with the proposed joint operation.

comment:4 Changed 2 years ago by sebastian

For reference, here is the CommonJS specification: http://wiki.commonjs.org/wiki/Modules/1.1#Module_Identifiers

And IMO this is exactly what we should implement.

comment:5 Changed 2 years ago by trev

I'm very inclined to drop our own system in favor of webpack at this point. This would have a number of advantages:

  • We would use a known-good solution instead of approximating the spec with our own code.
  • Webpack resolves modules at compile time, so we wouldn't need complicated run time path resolution code.
  • The modules to be included are determined automatically, we would merely have to specify paths (such as adblockpluscore) where modules should be searched for - assuming that we don't just use relative paths everywhere (../adblockpluscore/lib/filterClasses is lengthy but unambiguous).

There are disadvantages of course:

  • We would have to requires Node as a build dependency. Currently, it is only required for linting and docs generation.
  • We would need to install webpack somehow, it cannot currently be installed via our usual dependencies system. I can think of three possible solutions:
    • Requiring a global webpack installation (what we do for JSDoc right now), this is very awkward.
    • Adding package.json to repositories like adblockpluschrome and requiring npm install to be run before the build. This is only marginally better, with two dependencies systems competing now.
    • Adding the ability to install npm modules to ensure_dependencies.py. It's questionable whether the resulting complexity is justified.

comment:6 Changed 2 years ago by kzar

Sounds good to me, I'd be in favour of switching to webpack for this, though I'd prefer to add a package.json file to adblockpluschrome and specify the dependency there. That would also mean we could add more Node.js bits to the adblockpluschrome build process in the future, for example we could have eslint as a dependency and lint at buildtime too.

Last edited 2 years ago by kzar (previous) (diff)

comment:7 Changed 2 years ago by trev

The obvious disadvantage of this approach is: we would be specifying buildtools dependencies in adblockpluschrome. So if buildtools requires a newer webpack version in future, we would need to update the transitive dependency in adblockpluschrome. Also, buildtools code using webpack would have to look for the node_modules subdirectory in its parent directory.

Frankly, I think that we won't get around extending ensure_dependencies.py if we seriously want to go in this direction. We could treat npm like a special VCS, running npm ls --json to retrieve a "repository" status and npm install to install or update packages.

comment:8 Changed 2 years ago by kzar

Oh yea, good point. I forgot that webpack would be a dependency of buildtools not adblockpluschrome.

comment:9 Changed 2 years ago by trev

The other option would be putting package.json into buildtools repository. We could then make sure that ensure_dependencies.py runs npm install --only=production when updating a repository containing package.json. That should be simpler.

comment:10 Changed 2 years ago by kzar

Sounds good to me.

comment:11 Changed 2 years ago by sebastian

I'm undecided about using webpack at the current point. The dependency management seems to become unnecessarily complex (and inconvenient for the users of buildtools), if we mix up node and Python here. However, a while ago the idea to move buildtools completely from Python to node came up. But a hybrid solution seems quite awkward and complicated to me, in particular if it is just for the module system. On the other hand, the changes to our existing implementation in Python in order to make it more compatible with the spec don't seem too unreasonable.

comment:12 Changed 2 years ago by kzar

Well realistically a complete rewrite switching to Node.js is unlikely to happen, but if we start using Node.js for webpack perhaps we could gradually switch where it makes sense?

comment:13 Changed 2 years ago by trev

Note that we have this issue with JSDoc already. While most repository users won't need to build documentation, the solution as we have it right now is extremely awkward. The solution in comment:9 seems simple enough, yet allows combining our current dependencies mechanism and npm dependencies transparently. And it indeed allows a gradual migration of buildtools to Node.

comment:14 Changed 2 years ago by sebastian

Fair enough, I guess, since you and kzar seem to agree on this approach.

comment:15 Changed 2 years ago by kzar

  • Blocked By 5559 added

comment:16 Changed 2 years ago by kzar

  • Description modified (diff)
  • Summary changed from Add proper path support to our module system to Begin using webpack for script bundling in the Chrome packager

comment:17 follow-up: Changed 2 years ago by kzar

  • Owner set to kzar

I'll do this one I think Tristan since it's mostly just setting up webpack now and I recently did something very similar for the adblockpluscore tests.

comment:18 in reply to: ↑ 17 Changed 2 years ago by tlucas

Replying to kzar:

I'll do this one I think Tristan since it's mostly just setting up webpack now and I recently did something very similar for the adblockpluscore tests.

Alright - i'll look into #5559 ASAP and give you updates on it

comment:19 Changed 2 years ago by trev

  • Description modified (diff)

comment:20 Changed 2 years ago by hfiguiere

  • Blocking 5584 added

comment:21 Changed 2 years ago by hfiguiere

  • Cc hfiguiere added

comment:22 Changed 2 years ago by kzar

  • Blocking 5584 removed

comment:23 Changed 2 years ago by kzar

  • Blocked By 5596 added

comment:24 Changed 2 years ago by kzar

  • Description modified (diff)

comment:25 Changed 2 years ago by kzar

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

comment:26 Changed 2 years ago by sebastian

  • Blocked By 5751 added

comment:27 Changed 2 years ago by kzar

  • Blocking 5760 added

comment:28 Changed 2 years ago by kzar

  • Blocking 5761 added

comment:29 Changed 2 years ago by kzar

  • Blocking 5762 added

comment:30 Changed 2 years ago by kzar

  • Blocked By 5593 added

comment:31 Changed 2 years ago by kzar

  • Blocked By 5593 removed

comment:32 Changed 2 years ago by kzar

  • Blocked By 5751 removed

(Since we now hard code the resolve paths buildtools/lib is no longer a problem and therefore this issue is no longer blocked by #5751.)

comment:33 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 5535 - Replace our module system with webpack

comment:34 Changed 2 years ago by kzar

  • Blocking 5837 added

comment:35 Changed 2 years ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:36 Changed 2 years ago by kzar

  • Resolution fixed deleted
  • Review URL(s) modified (diff)
  • Status changed from closed to reopened

comment:37 Changed 2 years ago by kzar

  • Status changed from reopened to reviewing

comment:38 Changed 2 years ago by tlucas

  • Blocking 5383 added

comment:39 Changed 2 years ago by sebastian

I backed out the change. Apparently, the contents of package-lock.json depends on the npm version, causing a huge mess (#5856). Furthermore, I noticed that the bundled scripts are redundantly added to the build in their source form. The upcoming dependency update (#5837) is already complex enough, and I don't want to further delay it.

comment:40 Changed 2 years ago by sebastian

  • Blocking 5837 removed

comment:41 Changed 2 years ago by kzar

  • Blocked By 5856 added

comment:42 Changed 2 years ago by kzar

  • Review URL(s) modified (diff)

comment:43 Changed 2 years ago by kzar

  • Description modified (diff)

comment:44 Changed 2 years ago by tlucas

  • Blocking

comment:45 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 5535 - Replace our module system with webpack

comment:46 Changed 2 years ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:47 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 5535 - Fix a typo in the gecko build name

Note: See TracTickets for help on using tickets.