Skip to content

misc(publish): include the report bundle in npm package #13349

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.

Sign up for GitHub

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

Jump to bottom
Merged
paulirish merged 8 commits into from
Nov 11, 2021
Merged

misc(publish): include the report bundle in npm package #13349

paulirish merged 8 commits into from
Nov 11, 2021

Conversation

@paulirish
Copy link
Member

@paulirish paulirish commented Nov 10, 2021

to support folks like web.dev using our report renderer we need to include the new renderer API in our published assets

@paulirish paulirish requested a review from a team as a code owner November 10, 2021 00:15
@paulirish paulirish requested review from connorjclark and removed request for a team November 10, 2021 00:15
@google-cla google-cla bot added the cla: yes label Nov 10, 2021
connorjclark
connorjclark reviewed Nov 10, 2021
View reviewed changes
@connorjclark
Copy link
Collaborator

connorjclark commented Nov 10, 2021 •
edited
Loading

What's wrong with a client using lighthouse/report/renderer/report-renderer.js ? EDIT: oh, the function requires this file in the dist output. nvm

We could even provide an exports field in package.json so lighthouse/renderer would import report-renderer.js

@paulirish
Copy link
Member Author

paulirish commented Nov 10, 2021

We could even provide an exports field in package.json so lighthouse/renderer would import report-renderer.js

looks good. tried it out with a local web.dev (thank you build-pack) and we're good.

esm, too. ;)

connorjclark
connorjclark reviewed Nov 10, 2021
View reviewed changes
package.json Outdated
"exports": {
".": "./lighthouse-core/index.js",
"./renderer": "./dist/report/bundle.esm.js"
},
Copy link
Collaborator

@connorjclark connorjclark Nov 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

When exports is defined, it means only files that match one of these rules may be imported from the package. See the output from yarn build-devtools for how this breaks pubads:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './lighthouse-core/lib/i18n/i18n' is not defined by "exports" in /Users/runner/work/lighthouse/lighthouse/lighthouse/node_modules/lighthouse/package.json

Adding this line should fix it:

"./lighthouse-core/*": "./lighthouse-core/*"

@devnook devnook mentioned this pull request Nov 10, 2021
Closed
@paulirish paulirish added the 9.0 label Nov 10, 2021
@paulirish
Copy link
Member Author

paulirish commented Nov 11, 2021

@connorjclark can we merge as is? i certainly don't want to block v9 release with a "lets lock down our node exports" exercise. We've already found that it's not trivial with just one of our many downstream customers. :(

@paulirish paulirish changed the title misc(publish): include the report renderer bundle in npm package misc(publish): include the report bundle in npm package Nov 11, 2021
@paulirish paulirish merged commit f017815 into master Nov 11, 2021
@paulirish paulirish deleted the publishreportbundle branch November 11, 2021 20:13
@connorjclark connorjclark mentioned this pull request Nov 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@connorjclark connorjclark connorjclark approved these changes

Assignees

@connorjclark connorjclark

Labels

9.0 cla: yes waiting4reviewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@paulirish @connorjclark @devtools-bot