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

Can we reduce dependency count to make this actiually minimal ? Readme says minimal dependencies but actually has 165 dependencies which is absolutely insane. #48

Open
Raynos opened this issue Aug 23, 2023 · 13 comments

Comments

@Raynos
Copy link

Raynos commented Aug 23, 2023

  • string.prototype.replaceall seems like an excessive dependency to use here when we can use regex replace with the g parameter. ( https://stackoverflow.com/a/17606289 ) there are alternative implementations that take 4 lines instead of 30 npm packages.
  • readable-stream ; this seems excessive, it's a large package and a duplicate of require('stream')

Leaving minimist as is seems ok, but I'd also inline all the argument parsing and go for zero dependencies. But at least I understand the purpose of it.

@Raynos
Copy link
Author

Raynos commented Aug 23, 2023

raynos at system76-pc  
~/soundxyz/test123
$ npm ls --depth 9999 | wc -l
167

The test package claims to have minimal dependencies but has 167 transitive dependencies because string.prototype.replaceall is the literal definition of dependency hell and over engineering.

@Raynos
Copy link
Author

Raynos commented Aug 23, 2023

[email protected] /home/raynos/soundxyz/test123
└─┬ [email protected]
  ├── [email protected]
  ├─┬ [email protected]
  │ ├─┬ [email protected]
  │ │ └── [email protected]
  │ ├─┬ [email protected]
  │ │ ├── [email protected]
  │ │ └── [email protected]
  │ ├── [email protected]
  │ ├── [email protected]
  │ └─┬ [email protected]
  │   └── [email protected]
  └─┬ [email protected]
    ├─┬ [email protected]
    │ ├── [email protected]
    │ └── [email protected] deduped
    ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ │ └── [email protected] deduped
    │ └── [email protected]
    ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├── [email protected]
    │ ├── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├─┬ [email protected]
    │ │ │ └── [email protected] deduped
    │ │ └─┬ [email protected]
    │ │   └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected]
    │ ├── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ └── [email protected] deduped
    │ ├── [email protected] deduped
    │ ├── [email protected]
    │ ├── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └─┬ [email protected]
    │ │   ├── [email protected] deduped
    │ │   ├── [email protected] deduped
    │ │   └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├── [email protected]
    │ ├── [email protected]
    │ ├── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ └── [email protected] deduped
    │ ├── [email protected]
    │ ├── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected]
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├─┬ [email protected]
    │ │ │ └── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected] deduped
    │ │ └── [email protected] deduped
    │ ├─┬ [email protected]
    │ │ ├── [email protected] deduped
    │ │ ├── [email protected]
    │ │ ├── [email protected] deduped
    │ │ └─┬ [email protected]
    │ │   ├─┬ [email protected]
    │ │   │ └── [email protected] deduped
    │ │   ├─┬ [email protected]
    │ │   │ ├── [email protected] deduped
    │ │   │ └── [email protected] deduped
    │ │   ├─┬ [email protected]
    │ │   │ └── [email protected] deduped
    │ │   ├── [email protected] deduped
    │ │   └── [email protected] deduped
    │ └─┬ [email protected]
    │   ├── [email protected] deduped
    │   ├── [email protected] deduped
    │   ├── [email protected] deduped
    │   ├── [email protected] deduped
    │   └── [email protected] deduped
    ├─┬ [email protected]
    │ ├── [email protected] deduped
    │ ├── [email protected] deduped
    │ ├── [email protected] deduped
    │ └── [email protected] deduped
    ├── [email protected]
    └─┬ [email protected]
      ├── [email protected] deduped
      └─┬ [email protected]
        └── [email protected] deduped

@Raynos Raynos changed the title Can we reduce dependency count to make this more minimal ? Can we reduce dependency count to make this actiually minimal ? Readme says minimal dependencies but actually has 165 dependencies which is absolutely insane. Aug 23, 2023
@Raynos
Copy link
Author

Raynos commented Aug 23, 2023

I'm being a little bit unfair

aynos at system76-pc  
~/soundxyz/test123
$ npm ls --depth 9999 | grep -v deduped
[email protected] /home/raynos/soundxyz/test123
└─┬ [email protected]
  ├── [email protected]
  ├─┬ [email protected]
  │ ├─┬ [email protected]
  │ │ └── [email protected]
  │ ├─┬ [email protected]
  │ │ ├── [email protected]
  │ │ └── [email protected]
  │ ├── [email protected]
  │ ├── [email protected]
  │ └─┬ [email protected]
  │   └── [email protected]
  └─┬ [email protected]
    ├─┬ [email protected]
    │ ├── [email protected]
    ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ └── [email protected]
    ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├── [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ │ ├─┬ [email protected]
    │ │ └─┬ [email protected]
    │ ├─┬ [email protected]
    │ │ └── [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├── [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ │ └─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├── [email protected]
    │ ├── [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├── [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ │ └── [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ ├─┬ [email protected]
    │ │ ├── [email protected]
    │ │ └─┬ [email protected]
    │ │   ├─┬ [email protected]
    │ │   ├─┬ [email protected]
    │ │   ├─┬ [email protected]
    │ └─┬ [email protected]
    ├─┬ [email protected]
    ├── [email protected]
    └─┬ [email protected]
      └─┬ [email protected]

raynos at system76-pc  
~/soundxyz/test123
$ npm ls --depth 9999 | grep -v deduped | wc -l
69

It's only 67 dependencies, not 167.

@ljharb
Copy link
Member

ljharb commented Aug 23, 2023

util.parseArgs exists now, so that seems like a viable way to replace minimist, at least.

@Raynos
Copy link
Author

Raynos commented Aug 23, 2023

Oh wow i didn't know that, I've been using horrible horrible code for a while now

const IS_DRY_RUN = process.argv.includes('--dry-run', 1);

// or 
  const maxSizeArg = argv.find(x => x.startsWith('--max-size='))
  const maxSize = maxSizeArg && maxSizeArg.split('=')[1]

@Raynos
Copy link
Author

Raynos commented Aug 23, 2023

Although util.parseArgs does not match nodejs 14 so it breaks compat. Where as require('stream') is pretty stable since nodejs 14.

The readable-stream package was more a streams 1 / 2 / 3 ; for nodejs 0.8 0.10 0.12 4.0 artifact.

@juliangruber
Copy link
Member

juliangruber commented Aug 24, 2023

  • please refrain from judgements like "insane", we all have different ways to write programs
  • I agree the dependency tree of string.prototype.replaceall could be optimized. I have chosen it because i expect it to behave as close to the original as possible, with any kind of argument types provided
    • if we drop node 14 support (out of maintanance) we can remove it
    • otherwise, finding a replacement that is 100% correct could be tricky
  • +1 to using util.parseArgs, and creating a ponyfill for older node versions
  • we're using readable-stream because some imports reach deep into its files, impossible (or risky) with core
> require('stream/lib/internal/streams/operators')
Uncaught Error: Cannot find module 'stream/lib/internal/streams/operators'

@benjamingr
Copy link
Member

replaceAll exists Natively in Node.js 16+ which itself is soon EoL, we should just use the native versions probably.

@ljharb
Copy link
Member

ljharb commented Aug 24, 2023

@juliangruber re string.prototype.replaceall, i doubt it can be while retaining the same robustness, back compat, and correctness, but i'm certainly open to PRs.

@aduh95

This comment was marked as duplicate.

@Raynos
Copy link
Author

Raynos commented Aug 25, 2023

Great, then I see a path towards this package having zero dependencies. which is exciting.

@benjamingr
Copy link
Member

@Raynos now would be a good time to say that PRs are welcome :)

@Raynos
Copy link
Author

Raynos commented Aug 28, 2023

I'll open a PR if I get time or it becomes my problem.

Right now my problem is that I use ava and that's a big problem. using test would be a step in the right direction, need to implement a codemod to auto port all the tests 😢

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

No branches or pull requests

5 participants