search

jfmengels / node-elm-review

CLI for elm-review
https://package.elm-lang.org/packages/jfmengels/elm-review/latest/
BSD 3-Clause "New" or "Revised" License
48 stars 25 forks source link

Leaner Dependencies #223

Open lishaduck opened 1 month ago

lishaduck commented 1 month ago

Globbing's been been on the todo list for a bit, so figured I'd file an issue.

lydell commented 1 month ago

Regarding cross-spawn, there’s also this: https://github.com/lydell/elm-watch/pull/66. The key here is that we only need to support spawning the different elm tools, not stuff in general, which makes the problem space much smaller. Basically, the approach is to first try to spawn my-command and if that fails (on Windows only) try spawning my-command.cmd. I don’t remember why I haven’t merged that though…

lydell commented 1 month ago

cross-spawn, glob and chalk are also used in node-test-runner. I think there is a big overlap in users between node-test-runner and elm-review, so for the dependencies to actually get leaner for many people, we need to make upgrades there as well (which I have nothing against, I love fewer dependencies, the only road block is fear of breakage).

lishaduck commented 1 month ago

Regarding cross-spawn, there’s also this: lydell/elm-watch#66. The key here is that we only need to support spawning the different elm tools, not stuff in general, which makes the problem space much smaller. Basically, the approach is to first try to spawn my-command and if that fails (on Windows only) try spawning my-command.cmd. I don’t remember why I haven’t merged that though…

Hmm. You know, maybe a package just for spawning elm tools would be useful. It'd be lean b/c we know what we need to support, and could cut down on size, though not on deps #s. Then, node-elm-review, elm-watch, elm-test (, elm pages?), etc, could use it, and then npm would dedupe them. There's probably a few different deps for this between those packages. elm-optimize-level2 is in js. So's elm-coverage. Probably quite a few others. I'm not saying anything concrete here, but maybe we could all standardize our deps. I mean, there's node-elm-compiler vendored here, we could upstream some, etc. Get elm-doc-preview up to date. I think there were some peers conflicts there. Etc. You shouldn't need a ton in node_modules to create solid elm apps. Related: #136 (would actually increase size, I think, but that's a reason to make it smaller 🤷‍♂️).

cross-spawn, glob and chalk are also used in node-test-runner. I think there is a big overlap in users between node-test-runner and elm-review, so for the dependencies to actually get leaner for many people, we need to make upgrades there as well

I would think that there's probably a good deal of duplication. If this is actually going to help, it'd probably have more impact in elm-test (as elm-review is probably elss used).

(which I have nothing against, I love fewer dependencies, the only road block is fear of breakage).

Yeah. Maybe once I finish types here, I could go work on elm-test for a while. Switch to TS, drop some deps, etc.

jfmengels commented 1 month ago

I don't know if you've seen the e18e initiative/project, which aims to reduce the dependencies of the npm ecosystem.

I just saw that they have an ESLint plugin, which reports that we could replace rimraf quite easily: https://github.com/es-tooling/module-replacements/blob/main/docs/modules/rimraf.md

For globby, they suggested replacing it by fdir and picomatch, which they've done for instance in https://github.com/egoist/tsup/pull/1158. I don't know if it could replace glob for us.

I have never heard of picocolors, we could check it out yes.

lishaduck commented 1 month ago

I don't know if you've seen the e18e initiative/project, which aims to reduce the dependencies of the npm ecosystem.

I'd seen it a few weeks ago, and it popped up yesterday, which is where I got the idea to swap out chalk.

I just saw that they have an ESLint plugin, which reports that we could replace rimraf quite easily: https://github.com/es-tooling/module-replacements/blob/main/docs/modules/rimraf.md

For globby, they suggested replacing it by fdir and picomatch, which they've done for instance in egoist/tsup#1158. I don't know if it could replace glob for us.

I have never heard of picocolors, we could check it out yes.

Hmm. Ok, I'll take a look at those replacements a bit closer later.

SuperchupuDev commented 1 month ago

just my two cents:

lishaduck commented 1 month ago
  • globby can be replaced by tinyglobby, some replacement i published after struggling for weeks when trying to replace globby in tsup (two subdependencies)
  • glob can be replaced by fdir and picomatch, which is exactly what tinyglobby uses

Nice, thanks for that!

  • you can use undici for fetching if targeting node 16+ (or native fetch if node 18+)

We still support 14, and isn't fetch under a flag until 21?

SuperchupuDev commented 1 month ago

nope, fetch was added in like 17 point something and unflagged in 18.0.0

https://nodejs.org/en/blog/announcements/v18-release-announce#fetch-experimental

image

lishaduck commented 1 month ago

nope, fetch was added in like 17 point something and unflagged in 18.0.0

https://nodejs.org/en/blog/announcements/v18-release-announce#fetch-experimental

Ah, I see. It was experimental, but not under a flag. I think we try not to use even unflagged experimental APIs, but that's good to know nonetheless, thanks!

sindresorhus commented 3 weeks ago

Execa [...] and has some longstanding bugs

Execa has no known bugs. It's more likely @jsdevtools/ez-spawn has some bugs as it's not widely used and has not been updated for 4 years.

I'd seen it a few weeks ago, and it popped up yesterday, which is where I got the idea to swap out chalk.

I recommend reading this: https://github.com/chalk/chalk#why-not-switch-to-a-smaller-coloring-package

lishaduck commented 3 weeks ago

Execa has no known bugs.

I thought there were some, looks like I was wrong there.

It's more likely @jsdevtools/ez-spawn has some bugs as it's not widely used and has not been updated for 4 years.

cross-spawn hasn't been updated in 4 as well , and @jsdevtools/ez-spawn is smaller.[^1]

I recommend reading this

I did. I still think chalk wouldn't be deduped, we're still on an ancient version because this CJS. In addition, I didn't plan on getting to chalk for a while, as elm-test depends on it, and it doesn't make much of a difference.

I primarily want to drop glob or globby (we don't need 2), got (fetch is better), and fs-extra (already mostly gone #120). I'd like to drop rimraf too.

[^1]: I'm removing that one, execa is 1) ESM-only, and 2) depends on cross-spawn; @jsdevtools/ez-spawn isn't actually smaller. Always check you're sources.

lishaduck commented 3 weeks ago

In addition, your comment in the Chalk readme that

it [removing deps] might increase it [total node_modules size]

is correct, but misses the point. It's not that the node_modules of large applications will become smaller overnight, it's that the ecosystem's sizes become smaller over time. It's a long-term bet in the health of the JS ecosystem, in setting a new standard. Honestly, I don't even know if I think it'll work. But I believe in Elm, that's why I'm here. I want to make Elm the best experience around, and that's a long-term bet, not a bet that'll pay off overnight. Will Elm ever go mainstream? Probably not, that's not the aim anymore, but for those of us who've stayed, let's work at it.

The real problem lies with packages that have very deep dependency trees (for example, those including a lot of polyfills) ... It's better to focus on impactful changes rather than minor optimizations.

Amen. that's why this is far down on this list over at #169. I don't expect a major difference, so I'm not prioritizing it.

Signing off, @lishaduck.

I assume you're just global searching for Picocolors (or e18e), so I don't expect you to read this, but I wanted to clarify the goal of this issue anyway. If you do read this, I hope there are no hard feelings. I admire your work.