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

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

Merged
merged 1 commit into from
Jan 7, 2022

Conversation

brendankenny
Copy link
Member

Long ago (#5293) we substituted url-shim for the Node url module because robots-parser needed require('url').URL to give the WHATWG URL but the browserify polyfill for 'url' didn't have URL on it. In the meantime, the pubads plugin also started relying on that behavior, importing URL the same way. For some reason, the most popular rollup 'url' polyfills still don't have URL on them, so we've still been shoving url-shim in there.

However, there are other things on url-shim, and this can cause issues :)

We really only need the slimmest of polyfills for 'url', since only the URL property is needed, so this PR replaces it with one tiny version.

I also opened PRs in googleads/publisher-ads-lighthouse-plugin#325 and samclarke/robots-parser#23 to switch them both to the global URL, so we can remove this shim altogether when those land and are published.

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

// @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:

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

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.

None yet

4 participants