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

extension: expose URL shim #5293

Merged
merged 2 commits into from
May 23, 2018
Merged

extension: expose URL shim #5293

merged 2 commits into from
May 23, 2018

Conversation

patrickhulce
Copy link
Collaborator

Related Issues/PRs
fixes #5273

// browerify's url shim doesn't work with .URL in node_modules, so expose our own
// @see https://github.com/GoogleChrome/lighthouse/issues/5273
const pathToURLShim = require.resolve('../lighthouse-core/lib/url-shim.js');
bundle = bundle.require(pathToURLShim, {expose: 'url'});
Copy link
Member

Choose a reason for hiding this comment

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

where is this lowercase usage of it used? i'm not following what the fix is here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

inside robots-parser it does var URL = require('url').URL

@@ -132,6 +132,7 @@ class URLShim extends URL {
}
}

URLShim.URL = URL;
Copy link
Member

Choose a reason for hiding this comment

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

who uses URL.URL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

robots parser, see above :)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

well, this definitely only adds to the url-shim browserify confusion but I can't think of a better solution so LGTM :)

we do expose ourselves to some possible future bugs if we end up using a module that uses require('url').parse() or whatever, but there's not a great solution available and hopefully everyone will be using the global URL soon enough...

@paulirish paulirish added the 3.0 label May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SEO - Page is blocked from indexing - robots.txt
3 participants