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

report: convert to ES modules #12702

Merged
merged 86 commits into from
Jul 15, 2021
Merged

report: convert to ES modules #12702

merged 86 commits into from
Jul 15, 2021

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Jun 25, 2021

ref #12689, #12623

  • converts report/renderer and report/test/renderer to ES modules
  • adds package.json {type: 'module'} to a couple folders, to be removed when root package.json is esm
  • report-generator.js now reads standalone.js from disk, which is generated by yarn build-report. This file is not checked into source control, but will be published. moved to misc(build): add build step for report #12707
  • separate builds for each report entry
  • temporary: copy and convert to commonjs report/renderer/util.js to lighthouse-core/util-commonjs.js. Remove when ☂️ Convert to ES modules #12689 is done
  • temporary: add viewer.js report client that simply sets some renderer classes to window. Remove when viewer is converted to ES modules. Same for treemap

CDT CL: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/2939422
PSI CL: cl/377963333

@connorjclark connorjclark requested a review from a team as a code owner June 25, 2021 03:15
@connorjclark connorjclark requested review from patrickhulce and removed request for a team June 25, 2021 03:15
@google-cla google-cla bot added the cla: yes label Jun 25, 2021
@connorjclark connorjclark mentioned this pull request Jun 25, 2021
17 tasks
@@ -0,0 +1,16 @@
/**
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this file should be temporary... once viewer is es modules this just goes away.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

WDYT about breaking this up a smidge further? it seems like the build process pieces could be in place even without the esm conversion and separate quite cleanly without much effort?

I might be misestimating the level of effort required for that, so quick gut check first :)

lighthouse-core/runner.js Outdated Show resolved Hide resolved
@connorjclark
Copy link
Collaborator Author

CDT CL is fully passing its tests now. Once this PR merges we can land that and the PSI CL.

The CDT integration test is presently failing with this PR, which is expected. It will be passing again when the CDT CL lands. Then I'll re-enable it as a required check.

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