clients: use minimal 'url' polyfil instead of url-shim #13545
Conversation
| "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", |
There was a problem hiding this comment.
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;', |
There was a problem hiding this comment.
exporting globals is annoying. Open to nicer ways of doing this
There was a problem hiding this comment.
Hmm I was expecting this to cause issues in CI because we use fileURLToPath in root.js:
Line 24 in da73d4a
There was a problem hiding this comment.
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 :/
There was a problem hiding this comment.
robots parser already shipped a new version (3.0.0) with that PR. pubads didnt yet
4 participants
Long ago (#5293) we substituted
url-shimfor the Nodeurlmodule becauserobots-parserneededrequire('url').URLto give the WHATWGURLbut the browserify polyfill for'url'didn't haveURLon 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 haveURLon them, so we've still been shovingurl-shimin 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 theURLproperty 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.