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.

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

clients: use minimal 'url' polyfil instead of url-shim #13545

Merged
merged 1 commit into from
Jan 7, 2022
Merged

clients: use minimal 'url' polyfil instead of url-shim #13545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion build/build-bundle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ async function build(entryPath, distPath, opts = {minify: true}) {
entries: {
'debug': require.resolve('debug/src/browser.js'),
'lighthouse-logger': require.resolve('../lighthouse-logger/index.js'),
'url': require.resolve('../lighthouse-core/lib/url-shim.js'),
},
}),
rollupPlugins.shim({
Expand All @@ -152,6 +151,12 @@ async function build(entryPath, distPath, opts = {minify: true}) {
import Audit from '${require.resolve('../lighthouse-core/audits/audit.js')}';
export {Audit};
`,
// Most node 'url' polyfills don't include the WHATWG `URL` property, but
// that's all that's needed, so make a mini-polyfill.
// @see https://github.com/GoogleChrome/lighthouse/issues/5273
// TODO: remove when not needed for pubads (https://github.com/googleads/publisher-ads-lighthouse-plugin/pull/325)
// and robots-parser (https://github.com/samclarke/robots-parser/pull/23)
'url': 'export const URL = globalThis.URL;',
Copy link
Member Author

Choose a reason for hiding this comment

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

exporting globals is annoying. Open to nicer ways of doing this

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I was expecting this to cause issues in CI because we use fileURLToPath in root.js:

lighthouse/root.js

Line 24 in da73d4a

const dir = importMeta ? path.dirname(url.fileURLToPath(importMeta.url)) : __dirname;

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I was referring to in #13519 (comment), it doesn't work in the current version either. Either nothing is calling it or at least not calling it with an importMeta. If/when we do need that (if we don't transform it out of the bundle in a different way) we'll need to find a different polyfill because rollup-plugin-polyfill-node doesn't have fileURLToPath either :/

Copy link
Member

Choose a reason for hiding this comment

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

robots parser already shipped a new version (3.0.0) with that PR. pubads didnt yet

}),
rollupPlugins.json(),
rollupPlugins.inlineFs({verbose: false}),
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@
"resolve": "^1.20.0",
"rollup": "^2.52.7",
"rollup-plugin-node-resolve": "^5.2.0",
"rollup-plugin-polyfill-node": "^0.7.0",
"rollup-plugin-polyfill-node": "^0.8.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

this is only semi-related, it gets rid of the Circular dependency: polyfill-node.global.js -> polyfill-node.global.js warning when running yarn build-devtools (FredKSchott/rollup-plugin-polyfill-node#17). We still have the circular stream polyfill warnings though.

"rollup-plugin-replace": "^2.2.0",
"rollup-plugin-shim": "^1.0.0",
"rollup-plugin-terser": "^7.0.2",
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7150,10 +7150,10 @@ rollup-plugin-node-resolve@^5.2.0:
resolve "^1.11.1"
rollup-pluginutils "^2.8.1"

rollup-plugin-polyfill-node@^0.7.0:
version "0.7.0"
resolved "https://registry.yarnpkg.com/rollup-plugin-polyfill-node/-/rollup-plugin-polyfill-node-0.7.0.tgz#938e13278c98a582a4f8814975ddd26f90afddcc"
integrity sha512-iJLZDfvxcQh3SpC0OiYlZG9ik26aRM29hiC2sARbAPXYunB8rzW8GtVaWuJgiCtX1hNAo/OaYvVXfPp15fMs7g==
rollup-plugin-polyfill-node@^0.8.0:
version "0.8.0"
resolved "https://registry.yarnpkg.com/rollup-plugin-polyfill-node/-/rollup-plugin-polyfill-node-0.8.0.tgz#859c070822f5e38d221e5b4238cb34aa894c2b19"
integrity sha512-C4UeKedOmOBkB3FgR+z/v9kzRwV1Q/H8xWs1u1+CNe4XOV6hINfOrcO+TredKxYvopCmr+WKUSNsFUnD1RLHgQ==
dependencies:
"@rollup/plugin-inject" "^4.0.0"

Expand Down