Replace deprecated querystring component with builtin URLSearchParams API

webpack-contrib / webpack-hot-middleware

Webpack hot reloading you can attach to your own server

MIT License

2.34k stars 296 forks source link

Replace deprecated querystring component with builtin URLSearchParams API #416

Open foosbar opened 3 years ago

foosbar commented 3 years ago

Operating System: macOS 11.6.1
Node Version: 16.13.0
NPM Version: 8.1.0
webpack Version: 5.64.0
${package} Version: 2.25.1

Expected Behavior / Situation

Installing dependencies (npm install) for my project produces a deprecation warning for the querystring dependency.

npm WARN deprecated querystring@0.2.1: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.

Actual Behavior / Situation

Remove the warning by removing querystring as a dependency and using URLSearchParams API

Modification Proposal

Remove the querystring component from the project
Set the required minimum version of Node.js to >= 10.12.0 (picked this version since mocha has the same requirement - but Node.js 10 supports the full URLSearchParams API)
Update client.js to utilize this new API

The big question is can we limit webpack-hot-middleware to Node.js 10.12 or higher - webpack@5 requires 10.13 or higher.

glenjamin commented 3 years ago

The deprecation warning on that npm package is actively harmful imo.

The querystring API is considered Legacy. While it is still maintained, new code should use the API instead.

There is nothing about this message that suggests there is any real reason to stop using the existing API which has been stable for years.

That said, there iss an open PR for this already https://github.com/webpack-contrib/webpack-hot-middleware/pull/411

foosbar commented 3 years ago

Agreed @glenjamin, but deprecated dependencies typically stop getting support and using standard APIs for such a minor usecase would offer better long term support.

Thanks for pointing out the open PR - missed that one and was about to open another.

glenjamin commented 3 years ago

using standard APIs for such a minor usecase would offer better long term support.

Yeah, if this was a random 3rd party module I'd be more inclined to switch, but querystring has been a standard and stable part of Node.js since at least 0.10.x in 2013, if not longer!

I suspect that npm warning is annoying enough that it's going to be easier to merge the PR than to leave things as they are

dartess commented 2 years ago

@glenjamin hey! friendly ping:

That said, there iss an open PR for this already https://github.com/webpack-contrib/webpack-hot-middleware/pull/411

it's going to be easier to merge the PR than to leave things as they are

are there any plans or barriers to merge that PR?

glenjamin commented 2 years ago

The only barrier right now is that I never got around to it, and I really really really dislike that they've marked a core library as actively deprecated rather than just frozen.

There is literally nothing wrong with the current code - and it annoys me that someone has created this work for any maintainer that depends on querystring.

dartess commented 2 years ago

@glenjamin yeah, it's disappointing. I understand.

Contrariwise I think I can understand the author of that package. querystring is based on the corresponding module from Node.js. And he, in turn, is legacy now (https://nodejs.org/api/querystring.html). Thus, the package continues to keep parity with its original. Or maybe I just made it up myself.

Anyway, on the other hand, we are users of these packages who receive annoying warning that distract from the really important warnings.

glenjamin commented 2 years ago

Yeah, I should have some time tomorrow to take a look and get this sorted

Thanks for the prompt

glenjamin commented 2 years ago

This is now released in 2.25.2