Skip to content

tests(smokehouse): convert to ES modules#13046

Merged
connorjclark merged 12 commits into
masterfrom
esm-smokehouse
Sep 13, 2021
Merged

tests(smokehouse): convert to ES modules#13046
connorjclark merged 12 commits into
masterfrom
esm-smokehouse

Conversation

@connorjclark

Copy link
Copy Markdown
Collaborator

ref #13045

  • Converts lighthouse-cli/test/smokehose to esm.
  • Updates build-smokehouse-bundle to use rollup

@connorjclark connorjclark requested a review from a team as a code owner September 10, 2021 22:59
@connorjclark connorjclark requested review from patrickhulce and removed request for a team September 10, 2021 22:59
@google-cla google-cla Bot added the cla: yes label Sep 10, 2021
const lighthouse = require('../../fraggle-rock/api.js');
const puppeteer = require('puppeteer');
const StaticServer = require('../../../lighthouse-cli/test/fixtures/static-server.js').Server;
// TODO(esmodules): Node 14, 16 crash with `--experimental-vm-modules` if require and import

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodejs/node#39330 is back :(

Comment thread lighthouse-cli/test/smokehouse/frontends/smokehouse-bin.js Outdated
Comment thread lighthouse-cli/test/smokehouse/test-definitions/core-tests.js Outdated
Comment thread lighthouse-cli/test/smokehouse/test-definitions/forms/form-config.js Outdated
Comment thread package.json
Comment thread package.json Outdated
Comment thread package.json Outdated
"build-devtools": "yarn reset-link && node ./build/build-bundle.js clients/devtools-entry.js dist/lighthouse-dt-bundle.js && node ./build/build-dt-report-resources.js",
"build-smokehouse-bundle": "node ./build/build-smokehouse-bundle.js",
"build-lr": "yarn reset-link && node ./build/build-lightrider-bundles.js",
"build-lr": "yarn reset-link && node ./build/build-lightrider-bundles.js && rollup lighthouse-cli/test/fixtures/static-server.js -o dist/lightrider/static-server.js -p commonjs -p node-resolve -e mime-db,glob",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates build-lr to also bundle up static-server.js, necessary since it now imports non-core modules (And bundling it all up is simplest way to run it in google3).

Can you explain what changed (those deps were added two years ago)? And maybe just for my lack of rollup knowledge: why mime-db and not mime-types?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why mime-db and not mime-types?

Should be mime-types. I chose the wrong package name.


es-main was added, which is not in google3. mime-types and glob are in google3, so I made them externals here to keep the bundle smaller.

Also, due to how we currently roll the static server into google3 (copy/paste, basically), the ../../../root.js import is left as-is (which wouldn't work), and the code is string replaced to turn the import into a const. Since the file is now bundled, rollup takes care of that. Future imports form outside this file should be handled without further complication now that rollup is being used (sans an optional step of "is this dep in google3? ok, let's just make it external"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And due to overlooking while splitting out this PR, I forgot to make this file esm and actually use es-main.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I made them externals here to keep the bundle smaller.

Ah, got it, thanks. This was the part I was missing

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should I make a build-lh.sh script so I can leave this comment?

I prefer using rollup CLI where possible as it's pretty simple to read and write.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it's fine? I should have looked for the -e flag but got distracted by the node-resolve plugin instead. At the very least blame will find this conversation :)

Comment thread lighthouse-cli/test/smokehouse/frontends/smokehouse-bin.js Outdated
Comment thread lighthouse-cli/test/smokehouse/report-assert.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants