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

Add url dependency for browser compatibility #28

Closed
wants to merge 4 commits into from

Conversation

rvanasa
Copy link

@rvanasa rvanasa commented Aug 5, 2022

This package causes a variety of issues with recent versions of create-react-app and Vite projects due to the need for a polyfill of Node's url module. This PR adds the url npm package as a dependency to make this package work out-of-the-box in the latest version of Webpack and other front-end bundlers.

In the meantime, the npm package isomorphic-parse-github-url is available as a short-term solution.

This change passes all tests and works as expected in a production web application.

@@ -1,7 +1,7 @@
{
"name": "parse-github-url",
"name": "isomorphic-parse-github-url",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this part shouldn’t be in the PR.

@@ -33,6 +34,9 @@
"scripts": {
"test": "mocha"
},
"dependencies": {
"url": "^0.11.4"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this alone doesn’t do anything - requiring “url” always gets the core module, no matter what’s installed.

@rvanasa
Copy link
Author

rvanasa commented Nov 21, 2024

Apologies; I didn't realize that this was still open as a PR. Closing.

@rvanasa rvanasa closed this Nov 21, 2024
@ljharb
Copy link
Collaborator

ljharb commented Nov 21, 2024

@rvanasa i do hope you realize tho that the PR was ineffective, which includes your other package that you published?

@rvanasa
Copy link
Author

rvanasa commented Nov 21, 2024

Since bundlers often resolve packages differently from Node, the forked version of the library worked as intended. However, in recent versions of Vite, this workaround is no longer necessary.

@ljharb
Copy link
Collaborator

ljharb commented Nov 21, 2024

If a node module bundler does that, then it is broken, full stop.

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.

2 participants