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

Discourage use of certain npm packages #550

Open
13 tasks
jimmywarting opened this issue May 17, 2021 · 13 comments
Open
13 tasks

Discourage use of certain npm packages #550

jimmywarting opened this issue May 17, 2021 · 13 comments

Comments

@jimmywarting
Copy link
Contributor

jimmywarting commented May 17, 2021

  • inherits NodeJS docs recommends to use class extends instead
  • mkdirp node v10.12 mkdir supports recursive option
  • xtend, object-assign, extend-shallow use Object.assign or spread { ...obj }
  • rimraf node supports recursive option now
  • pad-left, pad-right, left-pad, right-pad, pad just use "".padStart() and "".padEnd()
  • safe-buffer, safer-buffer not needed anymore
  • array-flatten use flat instead or some other polyfill
  • request been deprecated
  • querystring is legacy, npm version got deprecated, can still require from node (use URLSearchParam instead)
  • co use async/await instead
  • windows-1252, string_decoder use TextDecoder instead
  • concat-map just use array.prototype.flatMap
  • buffer-alloc
@sindresorhus
Copy link
Member

sindresorhus commented May 17, 2021

I agree with all of these. Let's leave it open for a while to gather more ideas.

We can add them to https://github.com/xojs/eslint-config-xo/blob/1baef9fc745ee649fd1e03219331f22a9f5cc2e3/index.js#L235

@sindresorhus
Copy link
Member

  • object-assign - Same reason as xtend.

@jimmywarting
Copy link
Contributor Author

jimmywarting commented May 17, 2021

Added object-assign to the list.

I just wish to see fewer dependencies and smaller builds/footprint. JS is what makes the web slow - not large images

Unfortally not all packages gets deprecated - only discontinued - wish there was something like "Vote to deprecate this package" system in place like how you can vote to delete Q/A on StackOverflow... like why do even this 1000-packages exist?

@papb
Copy link

papb commented May 18, 2021

rimraf node supports recursive option now

Are you really sure Node.js' new option entirely replaces rimraf, including all the crazy hacks it has to make it work on Windows?

@jimmywarting
Copy link
Contributor Author

jimmywarting commented May 18, 2021

Are you really sure Node.js' new option entirely replaces rimraf, including all the crazy hacks it has to make it work on Windows?

I do not know about all the hacks but at least this exist in NodeJs and if there is some window bugs out there then i would have expect some of them to be fixed in v16

// Added in: v14.14.0
fs.rm(path {recursive: true}, callback) 
fsPromises.rm(path {recursive: true})

@sindresorhus
Copy link
Member

Eventually, I'd also like to discourage stream in favor of stream/web: https://github.com/nodejs/node/blob/master/doc/api/webstreams.md

@jimmywarting
Copy link
Contributor Author

jimmywarting commented Jul 1, 2021

Agree with you, Stream is going to be a tuff one to get ppl to stop using.
But i agree that it would be best to have something that works in Deno, Node & browser the same way and follows a spec'ed standard

for now i just use the async iterator part of node:streams and whatwg stream the same way

@jimmywarting
Copy link
Contributor Author

jimmywarting commented Jul 1, 2021

Saw this in a yarn log

warning @angular-devkit/build-angular > webpack-dev-server > url > querystring@0.2.0: The querystring is deprecated

Should we do something with url?

@sindresorhus
Copy link
Member

Should we do something with url?

There's no way for us to know whether it's the built-in url or the url package. I also don't think a lot of projects use the url package.

@Richienb
Copy link
Contributor

Richienb commented Oct 31, 2021

A list of discouraged packages should be compiled in a seperate repository than the main xo one so it can move faster and so there is a single centralised place to link to or add explainations and migration guides.

@sindresorhus
Copy link
Member

@Richienb That will just make it more difficult to maintain. Every added package is a breaking change as it causes a new lint error, so it would need an XO version bump anyway. We can consider doing that in the future if the list grows huge.

@jimmywarting
Copy link
Contributor Author

@Richienb That will just make it more difficult to maintain. Every added package is a breaking change as it causes a new lint error, so it would need an XO version bump anyway. We can consider doing that in the future if the list grows huge.

hmm, i think the list could change quite a lot. Would be annoying to have to bump major every time it change.
maybe it should just be a warning instead of an error?

Also what about checking sub dependencies also? doing so could encourage developers to send issues/pr upstream in the hope to reduce the number of warning messages

@sindresorhus
Copy link
Member

Would be annoying to have to bump major every time it change.
maybe it should just be a warning instead of an error?

Warnings don't really work. They are too easily ignored. And the user could have changed it to an error themselves, so we would still need a major bump (minor in XO's case because we're pre-1.0.0).

Also what about checking sub dependencies also? doing so could encourage developers to send issues/pr upstream in the hope to reduce the number of warning messages

That could potentially be the next step, but I think we should try out direct dependencies first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants