Opened on 08/18/2017 at 11:42:26 AM

Closed on 10/15/2017 at 01:05:25 PM

Last modified on 10/15/2017 at 01:56:14 PM

#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):

Description (last modified by kzar)


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/ does that work, making use of the buildtools/templates/modules.js.tmpl template.

Currently the logic in 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/ 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.

Attachments (0)

Change History (47)

comment:1 Changed on 08/18/2017 at 11:47:25 AM 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 on 08/18/2017 at 12:00:30 PM by kzar

  • Description modified (diff)

comment:3 Changed on 08/18/2017 at 12:38:00 PM 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 on 08/18/2017 at 03:09:30 PM by sebastian

For reference, here is the CommonJS specification:

And IMO this is exactly what we should implement.

comment:5 Changed on 08/21/2017 at 11:26:23 AM 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 It's questionable whether the resulting complexity is justified.

comment:6 Changed on 08/21/2017 at 11:31:29 AM 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 on 08/21/2017 at 11:41:29 AM by kzar

comment:7 Changed on 08/21/2017 at 11:53:51 AM 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 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 on 08/21/2017 at 12:08:36 PM by kzar

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

comment:9 Changed on 08/21/2017 at 12:10:49 PM by trev

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

comment:10 Changed on 08/21/2017 at 12:49:31 PM by kzar

Sounds good to me.

comment:11 Changed on 08/21/2017 at 12:53:47 PM 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 on 08/21/2017 at 12:58:54 PM 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 on 08/21/2017 at 01:37:37 PM 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 on 08/21/2017 at 03:29:54 PM by sebastian

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

comment:15 Changed on 08/23/2017 at 11:35:05 AM by kzar

  • Blocked By 5559 added

comment:16 Changed on 08/23/2017 at 02:40:51 PM 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 on 08/23/2017 at 02:42:52 PM 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 on 08/23/2017 at 06:04:46 PM 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 on 08/24/2017 at 10:01:14 AM by trev

  • Description modified (diff)

comment:20 Changed on 08/29/2017 at 03:05:12 AM by hfiguiere

  • Blocking 5584 added

comment:21 Changed on 08/29/2017 at 03:06:54 AM by hfiguiere

  • Cc hfiguiere added

comment:22 Changed on 08/29/2017 at 09:37:51 AM by kzar

  • Blocking 5584 removed

comment:23 Changed on 08/30/2017 at 02:24:33 PM by kzar

  • Blocked By 5596 added

comment:24 Changed on 08/31/2017 at 12:49:14 PM by kzar

  • Description modified (diff)

comment:25 Changed on 09/19/2017 at 02:29:27 PM by kzar

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

comment:26 Changed on 09/23/2017 at 06:28:39 PM by sebastian

  • Blocked By 5751 added

comment:27 Changed on 09/24/2017 at 09:10:23 AM by kzar

  • Blocking 5760 added

comment:28 Changed on 09/24/2017 at 09:28:42 AM by kzar

  • Blocking 5761 added

comment:29 Changed on 09/24/2017 at 09:35:07 AM by kzar

  • Blocking 5762 added

comment:30 Changed on 09/25/2017 at 06:01:39 PM by kzar

  • Blocked By 5593 added

comment:31 Changed on 10/09/2017 at 05:02:09 PM by kzar

  • Blocked By 5593 removed

comment:32 Changed on 10/10/2017 at 10:16:11 AM 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 on 10/10/2017 at 05:29:10 PM by abpbot

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

comment:34 Changed on 10/10/2017 at 05:39:53 PM by kzar

  • Blocking 5837 added

comment:35 Changed on 10/10/2017 at 05:40:14 PM by kzar

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

comment:36 Changed on 10/11/2017 at 05:16:01 PM by kzar

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

comment:37 Changed on 10/11/2017 at 05:16:27 PM by kzar

  • Status changed from reopened to reviewing

comment:38 Changed on 10/11/2017 at 07:47:06 PM by tlucas

  • Blocking 5383 added

comment:39 Changed on 10/11/2017 at 10:57:04 PM 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 on 10/11/2017 at 11:12:56 PM by sebastian

  • Blocking 5837 removed

comment:41 Changed on 10/12/2017 at 11:00:38 AM by kzar

  • Blocked By 5856 added

comment:42 Changed on 10/12/2017 at 11:58:19 AM by kzar

  • Review URL(s) modified (diff)

comment:43 Changed on 10/12/2017 at 02:49:00 PM by kzar

  • Description modified (diff)

comment:44 Changed on 10/13/2017 at 09:57:16 AM by tlucas

  • Blocking

comment:45 Changed on 10/15/2017 at 12:53:26 PM by abpbot

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

comment:46 Changed on 10/15/2017 at 01:05:25 PM by kzar

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

comment:47 Changed on 10/15/2017 at 01:56:14 PM by abpbot

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

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from kzar.
Note: See TracTickets for help on using tickets.