Begin using webpack for script bundling in the Chrome packager

[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.

[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?

[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.

[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.

[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.

[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.

[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.

[kzar]

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

[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.

[kzar]

Sounds good to me.

[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.

[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?

[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.

[sebastian]

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

[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.

[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

[kzar]

(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.)

[abpbot]

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

[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.

[abpbot]

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

[abpbot]

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