core: update robots-parser, extension: use browser-specific impl of url library #4875
Conversation
kdzwinel
requested review from
brendankenny,
patrickhulce and
paulirish
as code owners
lighthouse-extension/gulpfile.js
Outdated
| @@ -100,6 +100,8 @@ gulp.task('chromeManifest', () => { | |||
| function applyBrowserifyTransforms(bundle) { | |||
| // Fix an issue with imported speedline code that doesn't brfs well. | |||
| return bundle.transform('./fs-transform', {global: true}) | |||
| // Replace url module with url-shim | |||
| .transform('./url-transform', {global: true}) | |||
There was a problem hiding this comment.
is it not possible to get the bundle.require style working like we do for audits/gatherers? I don't know if it's possible really just curious :)
|
@patrickhulce oh my, things got interesting. Your question got me to dig deeper into browserify docs and it turns out that "browserify provides many browser-specific implementations of node core libraries" and that list includes… url module. So why it didn't work for us? lighthouse/lighthouse-extension/gulpfile.js Line 124 in f7efaa5 🙀Turns out that removing that one line is THE fix that we should have applied in the first place. Oh well, at least I learned about patch-package and browserify transforms 👨🎓 |
kdzwinel
changed the title
kdzwinel
changed the title
😱 nice sleuthing though! does this mean we can nuke the patch in this PR? I don't see it removed |
|
@patrickhulce good catch, removed 🗑 |
There was a problem hiding this comment.
cool beans this worked for me locally 👍 too bad you had that wild 🐦 chase @kdzwinel! appreciate the cleanup
browserify transform added to replacerequire('url')withrequire('path/to/url-shim')require('url')with a browser-specific implementationCloses #4825