-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core: convert to ES modules #14182
core: convert to ES modules #14182
Conversation
Co-authored-by: Adam Raine <6752989+adamraine@users.noreply.github.com>
Once I put a breakpoint down in lh-error.js module it became obvious that testdouble is somehow causing the module cache to get cleared between evaluation of a test file's static imports–and subsequent dynamic imports. This is a problem in files (like runner-test.js) which statically import a class later relied on to have a singleton value (LighthouseError, symbols) and assert (usually indirectly) that they are the same expected instance in a test. Ex:
minimal repro: import assert from 'assert';
import * as td from 'testdouble';
import {LighthouseError} from '../lib/lh-error.js';
td.replaceEsm('../gather/gatherers/stacks.js', undefined);
assert( (await import('../lib/lh-error.js')).LighthouseError === LighthouseError ); One obvious fix is to also dynamic import |
better repro: import assert from 'assert';
import 'quibble';
import {LighthouseError} from '../lib/lh-error.js';
assert( (await import('../lib/lh-error.js')).LighthouseError === LighthouseError );
|
Worked all day to produce this code, which fixes the unexpected "re-import" of non-mocked modules (the cause of multiple LighthouseError classes existing).
Just placing it here in case I have total amnesia tomorrow about what the fix was. Will do a proper confirmation by adding to quibble tomorrow, confirming tests don't break, and seeing if it really fixes the "minimal repro" I shared above. Related, but evidently not needed to fix the mock issues we've seen, is that I experimented with wrapping test files that use ES module mocks with a separate test file, to avoid the issues with mocking deps of modules imports by the test file. (That's why we have the ugly dynamic import hacks). The result is a much cleaner separation of mocks and the tests: https://github.com/GoogleChrome/lighthouse/blob/mock-wrapper-test-file/lighthouse-core/test/runner-test.js After this PR lands, I'll open a PR converting just runner-test.js to this new test format to facilitate discussion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM
@@ -282,15 +284,17 @@ function mockDriverSubmodules() { | |||
* @return {(...args: any[]) => void} | |||
*/ | |||
const get = (target, name) => { | |||
// @ts-expect-error: hack? What is going on here? Should we just remove the proxy stuff? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this ever figured out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment isn't new btw
ref #12689
This PR converts all of
lighthouse-core/
to ES modules.build/
__dirname
and__filename
are gone, replaced withimport.meta.url
. Thereplace
rollup bundle now drops the former and replaces the latter.module.createRequire
to get arequire
to userequire.resolve
, for getting a path to something inside a package. These resources end up being inlined by the inline-fs plugin, andrequire.resolve
disappears. This is fortunate, becausecreateRequire
can never work in bundled environments. With the help of terser the call tocreateRequire
is dropped entirely. Just in case it gets called somehow,createRequire
is shimmed with a specific error message. ex:lighthouse-core/lib/axe.js
build/plugins/inline-fs.js
supports__dirname
... there is no ESM equivalent, so I went withmoduleDir
which is the variable named used inrunner.js
.bundledModules
special variable inconfig-helpers.js
is now a map of promises, and each "dynamic module" is imported there.lighthouse-core/config
requireWrapper
- as before, this function first checksbundledModules
for an exact match. It now falls back toimport
with the provided path (or with.js
appended if no extension is present). This change is because the old behavior ofrequire
trying various extensions itself is not what ES import does. It looks only for what is given (sans any Node-runtime level aliases likepkg.exports
...). Once a module is found, if it has adefault
export that is what's returned. To support named exports in the future, the last thing this function does now is look for an export that has ameta
property, and returns that one.lighthouse-core/test/runner-test.js
,lighthouse-core/test/gather/gather-runner-test.js
. Also a couple tests were.skip
ed for further investigation (and marked with aTODO(esmodules)
comment)UIStrings
audits/
andgatherers
are due to__filename
being replaced withimport.meta.url
. To make this a simple change,createIcuMessageFn
replaces thefile://
prefix found onimport.meta.url
.Misc
td.replace
replaced withtd.replaceEsm
lighthouse-core/index.cjs
added so CJS users can continue to require lighthouse with minimal effort. What cannot be supported any longer are importing Audit/Gatherer subclasses for custom audits/gatherers.stuff for later
lighthouse-core/util-commonjs.cjs
import/order
lint pass