defunctzombie / node-url

node.js core url module as a module
MIT License
375 stars 96 forks source link

`querystring` 0.2.0 is deprecated #57

Closed rtritto closed 1 year ago

rtritto commented 2 years ago

Dependency querystring 0.2.0 should be removed because it is a deprecated and legacy dependency. Should use the URLSearchParams API instead.

Steps to reproduce

  1. yarn init -y
  2. yarn set version berry
  3. yarn add url
  4. See log

Results

Actual

➤ YN0000: ┌ Resolution step
➤ YN0061: │ querystring@npm:0.2.0 is deprecated: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
➤ YN0000: └ Completed in 0s 979ms
➤ YN0000: ┌ Fetch step
➤ YN0013: │ punycode@npm:1.3.2 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ querystring@npm:0.2.0 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ url@npm:0.11.0 can't be found in the cache and will be fetched from the remote registry
➤ YN0000: └ Completed in 0s 239ms
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed

Expected

No yarn warnings in log.

ljharb commented 2 years ago

That API doesn't work in old enough node versions to ever be viable for this module, and "considered legacy" is a very insignificant reason to switch off something.

Certainly we should switch from querystring to something like qs, though, for a number of other reasons.

jimmywarting commented 2 years ago

qs and querystring should both be discouraged b/c of it's inconsistency

const obj = querystring.parse('a=a&abc=x&abc=y') // { a: 'a', abc: ['x', 'y'] }

if you build a api and you rely on something to be a object then it can easily break if you just add a 2nd query param with the same key for it to break if it's unhandled in your code - something better is just to always return everything as a array, no matter if it's one item. Better yet is to use URLSearchParam to do the parsing and stick to native stuff that have a better decoding (and dose not require lots of code)

Even better don't use npm's or nodejs builting url, just use the global URL instead. node-url is the only package that should be allowed to use querystring for compatibility reasons. i think everyone should stop using require('url') and just use the global instance instead.

Maybe node-url should just stop depending on querystring and inline the hole package instead.

ljharb commented 2 years ago

@jimmywarting that's only with the default array format - brackets is actually the more common one, and doesn't suffer from this issue.

ljharb commented 2 years ago

Requiring the global URL is a breaking change that will never be an option.

jimmywarting commented 2 years ago

I didn't mean you should require the global URL, i only meant that developer should stop depending on this package... and use the global URL & URLSearchParam instead - you are basically required to keep the querystring to match nodejs core module

ljharb commented 2 years ago

ah. that may be an option for some users, sure, but lots of things depend on node's URL core module and won't ever be updated, and work fine, and they should be able to be used in a browser.

rtritto commented 2 years ago

@ljharb an alternative is to create a new major version to remove/replace/update (breaking change) all the deprecated/legacy dependencies. By the way, with different reasons, you can maintain both versions (current and new major). In my opinion, a dependency should have a consistency with all its dev/peer dependencies.

ljharb commented 2 years ago

@rtritto why would anyone sign up to maintain an additional simultaneous release line, unpaid, for virtually no actual benefit?

rtritto commented 2 years ago

It's only one viable way that depends on purposes. Use one main release is preferred (obviously).

fregante commented 1 year ago

@ljharb would you consider inlining the package to avoid the deprecation warning as suggested by @jimmywarting? Being deprecated, it will never need updates anymore.

Does using bundledDependencies avoid the warning on npm install? Because that would be the easiest fix possible.

Since this package is being used as a "browserify" polyfill for subdependencies (e.g. via https://github.com/Richienb/node-polyfill-webpack-plugin), switching to URL constructor isn't possible.

ljharb commented 1 year ago

@fregante inlining is a workable but hacky solution; I’d be happy to switch it to use qs tho.

ljharb commented 1 year ago

Closed by #62.