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

fix: conflict between browser URL object and Node URL object #3061

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

billybonks
Copy link
Contributor

I am running this package using electron, what i noticed was that due to the fact that the lines between node and browser environments become a bit blurred, the URL class that was being used was the one defined by the browser and not node. By making an explicit require it ensures the correct Class is used.

While creating a test for this would be difficuilt i think adding an eslint rule to stop using globally defined objects and require imports instead would resolve issues like this in the future

I am running this package using electron, what i noticed was that due to
the fact that the lines between node and browser environments become a
bit blurred, the URL class that was being used was the one defined by
the browser and not node. By making an explicit require it ensures the
correct Class is used.

While creating a test for this would be difficuilt i think adding an
eslint rule to stop using globally defined objects and require imports
instead would resolve issues like this in the future
@charmander
Copy link
Collaborator

What impact does the choice of URL implementation have in Electron?

@billybonks
Copy link
Contributor Author

@charmander if it uses the node version of URL it works great

image

and if you run the same in the browser it does not get parsed in the same way

image

which causes the connection string code not to work.

@boromisp
Copy link
Contributor

Probably related: #3032

@brianc
Copy link
Owner

brianc commented Sep 15, 2023

cool this seems good to me!

@brianc brianc merged commit d21cc09 into brianc:master Sep 15, 2023
15 checks passed
@zacharymarshal
Copy link

What would be the best way for me to get these changes right now? I am running into this same issue right now.

Thanks so much

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.

5 participants