Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

☂️ Convert to ES modules #12689

Closed
15 of 17 tasks
connorjclark opened this issue Jun 24, 2021 · 3 comments
Closed
15 of 17 tasks

☂️ Convert to ES modules #12689

connorjclark opened this issue Jun 24, 2021 · 3 comments

Comments

@connorjclark
Copy link
Collaborator

connorjclark commented Jun 24, 2021

Not breaking changes:

Breaking changes, wait for v10:

doc
(below is just some repeated info)


Changing CLI to esmodules isn't breaking?

Nope–the interface for the CLI is via the command line, not programmatic.

How to breakup work?

Reminder: ES modules can import commonjs, but the inverse is not possible (not without doing async imports). Therefore chunks of code with no dependents should be converted first.

Scripts and tests use modules from everywhere, so those should be converted first.

The report renderer, viewer, treemap, extension, CLI, scripts and tests can all be done independently, one at a time as they have no dependents (also mostly true for individual tests and scripts, but the dependency chains there are small).

Core is a big beast, and has the hardest problems to work out (I'm thinking of the config dynamic module loading code). The CLI depends on Core, so CLI must be converted first. Once lighthouse-core/runner.js and lighthouse-core/index.js is converted, and the config module loading is worked out, should be able to do each folder in lighthouse-core individually (but at this point, with everything else done, it may be reasonable to do all of it at once). See https://gist.github.com/connorjclark/22105a01b471811d9c5ccf989db3059d for part of the dependency list (doesn't include any dynamic modules).

The package.json type field is what enables esmodules (we don't want to do .mjs), and luckily the nearest package.json file is used, so during the transition we can just write {type: "module"} files where necessary.

Lighthouse Logger

This will be cumbersome since it is its own package. Should we move this into Lighthouse proper as lighthouse-core/lib/logger? We could defer updating the npm package until necessary, and drop the dependency on lighthouse-logger; that should allow us to convert this code w/o a breaking change.

EDIT: turns out this is simpler than all that. see #13720

Read more

https://electerious.medium.com/from-commonjs-to-es-modules-how-to-modernize-your-node-js-app-ad8cdd4fb662

@brendankenny
Copy link
Member

jestjs/jest#10025 ("jest.mock does not mock an ES module without Babel") seems like it will likely be an issue for us on the testing side. A draft PR is up to fix it (jestjs/jest#10976), but has been open for seven months and doesn't look to be a priority for them right now.

@connorjclark
Copy link
Collaborator Author

connorjclark commented May 17, 2022

esm-core branch unit test segfaults atm: https://github.com/GoogleChrome/lighthouse/runs/6461306971?check_suite_focus=true#step:13:1378 (this doesn't happen for me locally)

May be node vm dynamic import crbug, but also could be c8?

note: there is a non-segfaulty v8 bug too (Test environment has been torn down) but that is handled with this fancy script: https://github.com/GoogleChrome/lighthouse/blob/23b4add7bb59f1fbaad2c3d0c7278b78c6e01616/lighthouse-core/scripts/run-jest.sh

@connorjclark
Copy link
Collaborator Author

I'd say this is pretty much all done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants