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

Leaner Dependencies #223

Open
4 of 10 tasks
Tracked by #169
lishaduck opened this issue Jul 19, 2024 · 20 comments
Open
4 of 10 tasks
Tracked by #169

Leaner Dependencies #223

lishaduck opened this issue Jul 19, 2024 · 20 comments

Comments

@lishaduck
Copy link
Contributor

lishaduck commented Jul 19, 2024

  • We use (node-)glob, globby, and (as a dependency of globby) fast-glob. We ought to switch to tinyglobby
    • Replace globby with tinyglobby
    • Replace glob with tinnyglobby
  • Apparently e18e has recently been switching projects off of Chalk in favor of Picocolors
  • node-fetch-native is much leaner than node-fetch (though we use got, don't we?), and defaults to native fetch in Node 21+
  • We're also using cross-spawn, e18e reccomends tinyexec instead
  • e18e reccomends moving off ora in favor of nanospinner
  • Replace fs-extra #120
  • Use native rm over rimraf
  • Drop uglify-js

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

@lishaduck lishaduck mentioned this issue Jul 19, 2024
23 tasks
@lydell
Copy link
Contributor

lydell commented Jul 19, 2024

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…

@lydell
Copy link
Contributor

lydell commented Jul 19, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

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 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.

@lishaduck
Copy link
Contributor Author

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
Copy link

SuperchupuDev commented Jul 22, 2024

just my two cents:

  • 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
  • you can use undici for fetching if targeting node 16+ (or native fetch if node 18+)

@lishaduck
Copy link
Contributor Author

  • 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
Copy link

SuperchupuDev commented Jul 22, 2024

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
Copy link
Contributor Author

lishaduck commented Jul 22, 2024

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
Copy link

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
Copy link
Contributor Author

lishaduck commented Aug 14, 2024

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 is 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.

Footnotes

  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 your sources.

@lishaduck
Copy link
Contributor Author

lishaduck commented Aug 14, 2024

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.

@lishaduck
Copy link
Contributor Author

lishaduck commented Sep 23, 2024

@jfmengels, do we actually depend on uglify-js? I realized we don't use it anywhere.

@lishaduck
Copy link
Contributor Author

Oh, tinyglobby doesn't (actually) support Node v14, it seems: thecodrr/fdir#116 😞
I don't see any issues (open or closed) in tinyglobby about it, but I also don't see a matrix in CI, so 🤷‍♂️

@SuperchupuDev
Copy link

SuperchupuDev commented Oct 30, 2024

tinyglobby tries to support node >=12, as fdir doesn't use any node 14 exclusive features, but i don't run tests on node 12 due to test runner incompatibilities (fdir doesn't either for the same reason iirc). can you check?

@lishaduck
Copy link
Contributor Author

Our tests run on 16, not 14, which is why I asked. I'll see if I can get them to run on 14 though.

@lishaduck
Copy link
Contributor Author

@SuperchupuDev, you'll be pleased to hear that it workses!

@lishaduck
Copy link
Contributor Author

@jfmengels, what's your policy wrt experimental features? E.g., if cp was stabilized in v22, can we use hindsight and say it's ok to use assuming there were no breaking changes between v14 & now?

@jfmengels
Copy link
Owner

@lishaduck If the feature works well in the versions I support, then I'm okay with using them if they stabilized later. I would only be worried that they didn't work as well in earlier versions we intend to support, but if they work well, I'm fine with that.
If they haven't become stable in the future, then I'm less sure. I think I'd be okay with it, but to be checked on a case by case basis maybe?

@lishaduck
Copy link
Contributor Author

If the feature works well in the versions I support, then I'm okay with using them if they stabilized later.

👍 (#120 should be easy-peasy then!)

I would only be worried that they didn't work as well in earlier versions we intend to support,

Yeah. If they don't work well, we'd need to polyfill it, and at that point, just use the library.

but if they work well, I'm fine with that. If they haven't become stable in the future, then I'm less sure. I think I'd be okay with it, but to be checked on a case by case basis maybe?

I personally wouldn't want to be to blame if something breaks, so I wasn't planning on it.

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