1780074 - [meta] Migrate newtab code to ES modules

Status: RESOLVED FIXED

(Firefox :: New Tab Page, task, P2)

Description

[Mark Banner (:standard8)]

We should migrate the jsm files under browser/components/newtab to be ES modules.

This bug will handle the majority of the files for newtab, I'll file follow-ups for the remainder.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1780074 - Migrate browser/components/newtab to ESM. r?daleharvey

Depends on D152058

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1780074 - Migrate newtab consumers to use ESM imports directly. r?daleharvey

Depends on D152113

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3bc739f7de43
Migrate browser/components/newtab to ESM. r=daleharvey
https://hg.mozilla.org/integration/autoland/rev/a13d3939b98a
Migrate newtab consumers to use ESM imports directly. r=daleharvey,application-update-reviewers,nalexander

Comment 4

[Iulian Moraru]

Backed out for causing multiple failures.

Push with failures

Failure log for node newtab
Failure log for failed TV job on test_permmanager_migrate_7-8.js
Failure log for failed TV job on browser_newtab_trigger.js
Failure log for failed TV job on browser_default_bookmark_toolbar_visibility.js
Failure log for failed TV job on browser_all_files_referenced.js
Failure log for failed TV job on browser_ext_urlbar.js

Failure log for failed node newtab job on the backout push

Backout link

Comment 5

[Mark Banner (:standard8)]

Attachment: WIP: Bug 1780074 - Add babel plugin files for newtab transforms to the repository.

Comment 6

[Mark Banner (:standard8)]

Attachment: WIP: Bug 1780074 - Use the newly added babel plugins in newtab code.

Depends on D152557

Comment 7

[Mark Banner (:standard8)]

Attachment: WIP: Bug 1780074 - Add ES import handling to jsm-to-esmodules-plugin.

Depends on D152558

Comment 8

[Mark Banner (:standard8)]

Attachment: WIP: Bug 1780074 - Start modifying configs to for newtab tests.

Depends on D152114

Comment 9

[Mark Banner (:standard8)]

Dan/Ed, I think I'm going to need some help with fixing the tests here.

• The top of the stack is D152561, you should hopefully be able to apply it cleanly to the latest m-c.
• I have migrated all the modules to be .sys.mjs apart from the ones in lib/PersonalityProvider/ since they are used in a worker where we don't support ES modules yet (follow-up to be filed).
• The webpack build process works, and xpcshell tests/mochitests pass.
• The unit tests fail.

The unit tests seem to be failing due to a mixture of not liking the ES modules, and then not translating resource uris. I've just tried using @babel/plugin-transform-modules-commonjs in the Karma config, but that then complains about the system mjs modules not having exports, even though they clearly do have exports...

I'd appreciate some help here, as I'm a bit lost as to what's going on / the best way to fix this.

Comment 11

[Ed Lee :Mardak]

I finally got the stack applied and there were some conflicts with recent additions of imports (e.g., bug 1774471 DSCard.test.jsx and bug 1779636 DSLinkMenu.jsx) and context changing (manually git apply -C1 to reduce required context). I do see errors such as…

ERROR in ./lib/HighlightsFeed.sys.mjs (./loaders/inject-loader.js!./lib/HighlightsFeed.sys.mjs) 9:0
Module parse failed: 'import' and 'export' may only appear at the top level (9:0)
File was processed with these loaders:
 * ./loaders/inject-loader.js
 * ./node_modules/babel-loader/lib/index.js
You may need an additional loader to handle the result of these loaders.
| "use strict";
| 
> import { actionTypes as at } from "common/Actions.sys.mjs";
| import { shortURL } from "lib/ShortURL.sys.mjs";
| import { SectionsManager } from "lib/SectionsManager.sys.mjs";
 @ ./test/unit/lib/HighlightsFeed.test.js 6:0-57 88:8-16
 @ ./test/unit/ sync \.test\.jsx?$ ./lib/HighlightsFeed.test.js test/unit/lib/HighlightsFeed.test.js
 @ ./test/unit/unit-entry.js 23:12-55

Where I believe it's because of the import injector from "inject!lib/HighlightsFeed.jsm"; https://searchfox.org/mozilla-central/rev/5c04fc7016eb7f52cf835d482f1125c8f139c959/browser/components/newtab/test/unit/lib/HighlightsFeed.test.js#6

effectively wrapping the text of the original HighlightsFeed.sys.mjs into dynamically generated commonjs module that invokes the wrapped code which now has export not at the top level.

loader-utils was originally added long ago in https://github.com/mozilla/activity-stream/commit/448593700b3e2e9f0eed2a1db0afe39050580cf9

The purpose of the injector is to pass in a custom object with stubs instead of HighlightsFeed importing the normal code at say lib/ShortURL.sys.mjs.

k88hudson, any suggestions on if we still need this with things converting to esmodules or a different approach? I suppose there might need to be some actual esm to commonjs conversion?

Comment 12

[Ed Lee :Mardak]

Copying this comment from https://phabricator.services.mozilla.com/D152561#inline-852044 as it's in draft (and those don't usually send notifications?)

I noticed that for some reason, files like ASRouterParent https://searchfox.org/mozilla-central/rev/5c04fc7016eb7f52cf835d482f1125c8f139c959/browser/components/newtab/actors/ASRouterParent.jsm#10-15

that have a top level const VariableDeclaration as the first thing end up only having that as the one and only node in nodes resulting in errors like…

UnhandledSchemeError: Reading from "resource://activity-stream/lib/ASRouterNewTabHook.sys.mjs" is not handled by plugins (Unhandled scheme).`

because it doesn't find the ImportDeclaration to replace.

In this specific ASRouterParent file, if I move the const to below the imports, it's able to detect and replace the import path. However, other files have const lazy = {}; at the top.

Comment 13

[Ed Lee :Mardak]

Er. Nevermind comment 12. I misread the code but it's returning early once it hits a non-import instead of processing all imports.

Comment 14

[Mark Banner (:standard8)]

I've figured out a way to start being able to convert some of the common modules across to ES modules, and continue to have the bundling and test processes working - I've moved that part out to bug 1808202.

There's still more work to be done though. Transitioning to ES modules seems possible, but the injector is the issue (as Ed mentions in comment 11) - as that needs to inject custom modules in place of other modules, by wrapping the appropriate modules and injecting them into the source. Unfortunately I don't think we can use sinon's mock functions here, as these are modules imported into other modules, not the module under test.

So the next step after bug 1808202, is probably to investigate something that will work for the injector for mjs files.

I am hoping that once the whole transition is done, we might be able to get rid of some of the transitions to common js format, and have only ES module format.

Comment 15

[Mark Banner (:standard8)]

With the landing of bug 1815224, I think that potentially unlocks conversion of more modules reasonably easily.

I'm not currently planning on working on these, so unassigning myself. I am happy to advance/consult with to help with the transition. I just don't really have time to do it myself.

As part of that, here's the current status:

• The babel-plugin-jsm-to-commonjs plugin has been moved in-tree and now supports ChromeUtils.importESModules. iirc, this means that when a file is injected into tests using the injector, the imports will be replaced by require. Webpack then manages importing of the ES module via the require statement.
• The files in browser/components/newtab/actors/ and browser/components/newtab/common/ have been converted to ES modules.

I think the next steps would be:

1. Migrate most of the browser/components/newtab/lib modules.

• I think it should generally work as long as the module is not loaded in tests via the injector.
• It may be easier to start with modules that don't import modules themselves (e.g. LinksCache.jsm).
• This should be fairly automated - ./mach esmify --convert can convert single modules (though I'd recommend doing blocks of them together to save multiple patches), and ./mach esmify --import will convert most of the imports - though the ones in the content files will need updating manually.
• This patch is probably a good example of what a single-import conversion would look like.

1. For the modules that are loaded in tests via the injector, I think there may need to be some additional work - either to avoid the need for the injector itself (e.g. maybe sinon's mocks?), or write a ES module -> ES module plugin that handles the necessary parts for the injector to work.

Comment 16

[kpatenio]

Unassigning myself from the main ticket, which I'm now labelling as a meta bug.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

All dependencies closed. We're done here.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

(The last two JSM's still in the newtab component folder are being moved as part of bug 1877196)