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

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

Merged
merged 8 commits into from
Nov 11, 2021

Conversation

paulirish
Copy link
Member

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
.npmignore Outdated Show resolved Hide resolved
@connorjclark
Copy link
Collaborator

connorjclark commented Nov 10, 2021

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

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. ;)

package.json Outdated
"exports": {
".": "./lighthouse-core/index.js",
"./renderer": "./dist/report/bundle.esm.js"
},
Copy link
Collaborator

@connorjclark connorjclark Nov 10, 2021

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/*"

@paulirish
Copy link
Member Author

@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
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.

3 participants