search

GoogleChromeLabs / native-url

Node's url module implemented using the built-in URL API.
https://npm.im/native-url
Apache License 2.0
284 stars 11 forks source link

Swap `querystring` with `query-string` #19

Closed vladimyr closed 4 years ago

vladimyr commented 4 years ago

Swap query parser/formatter package & upgrading dev dependencies.

developit commented 4 years ago

Hiya - can you clarify why this is beneficial?

vladimyr commented 4 years ago

First I'm honestly flattered that you are reviewing my PR.

Short version: I'm an idiot 🤦‍♂

Longer version: I saw microbundle log saying:

Creating a browser bundle that depends on Node.js built-in module ('querystring'). You might need to include https://www.npmjs.com/package/rollup-plugin-node-builtins

and thought, well it doesn't bundle querystring instead uses it as external dep listed in package.json dependencies section. But querystring is 7 yrs old which is like ages in js world and it would probably be better to swap it with regularly maintained and up to date library like Sindre's query-string. What I failed to understand is that when running inside node external dependency will be shadowed by native querystring anyway which is a far better option than using external and potentially non-compliant implementation. But I already did the PR because I was typing faster than thinking 🤦‍♂️

The rest of PR is just bumping dev dependencies or me impersonating @dependabot and should be ok as is? I mean, no particular gains just keeping stuff up to date.

So do you want me to close it or to convert it to dev dependencies upgrade effort by removing querystring related part?