remix-run / remix-website

347 stars 79 forks source link

Bug: npm let us use undeclared (in package.json) dependencies #166

Closed fernandocanizo closed 9 months ago

fernandocanizo commented 9 months ago

Is there a serious showstopper reason to not be using pnpm yet? (jump to the end to see about the bug from the title of this issue)

npm

First run: ~55 seconds

$ time npm ci

added 1261 packages, and audited 1262 packages in 54s

10 vulnerabilities (9 moderate, 1 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.

real    0m54,574s
user    0m18,137s
sys     0m15,546s

Second run: ~20 seconds

$ time npm ci
[redacted...]
real    0m19,637s
user    0m11,526s
sys     0m8,368s

pnpm

First run: ~40 seconds

$ time pnpm i
Packages: +1168
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Progress: resolved 1232, reused 1167, downloaded 0, added 1168, done

[redacted...]

Done in 39.5s

real    0m39,637s
user    0m16,913s
sys     0m14,826s

Second run: ~3 seconds

$ time pnpm i
[redacted...]
Done in 2.7s

real    0m2,802s
user    0m8,708s
sys     0m2,670s

yarn

First run: ~113 seconds

$ time yarn install
yarn install v1.22.21
info No lockfile found.
[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
[4/5] Linking dependencies...
warning "@docsearch/react > @algolia/autocomplete-preset-algolia@1.9.3" has unmet peer dependency "@algolia/client-search@>= 4.9.1 < 6".
warning "@docsearch/react > @algolia/autocomplete-core > @algolia/autocomplete-plugin-algolia-insights@1.9.3" has unmet peer dependency "search-insights@>= 1 < 3".
warning "@docsearch/react > @algolia/autocomplete-core > @algolia/autocomplete-shared@1.9.3" has unmet peer dependency "@algolia/client-search@>= 4.9.1 < 6".
warning " > @remix-run/v1-meta@0.1.3" has unmet peer dependency "@remix-run/server-runtime@^1.15.0 || ^2.0.0".
[5/5] Building fresh packages...
success Saved lockfile.
Done in 112.08s.

real    1m52,302s
user    0m17,545s
sys     0m4,605s

Second run: ~7 seconds

$ time yarn install

Done in 6.84s.

real    0m7,085s
user    0m8,749s
sys     0m10,127s

Other considerations

Error: Cannot find module 'react-router-dom' imported from 'app/routes/docs.$lang.$ref.tsx'

This happens because the way both yarn and npm resolve peer dependencies, which is wrong, as it let us use packages that we haven't explicitly declared in our package.json.

I vote we jump head first to start using pnpm. If no arguments against are given, I'll do the PR.

brookslybrand commented 9 months ago

Thanks @fernandocanizo for raising this and adding all of the benchmarks, that was very helpful.

While I'm not opposed to pnpm in general, for me this is a bit of a "if it ain't broke don't fix it" situation. While faster installations might be slightly nice the first time you clone the repo and for our deploy script, to me it's not worth switching package managers over. The main reason being that we know npm works consistently in this repo and have yet to have someone run into a problem. With pnpm that's less proven, and so while it may work on yours and my machine, it easily could break on someone else's, or when we add some library.

I think going with the standard of npm for a repo setup as simple as this is the safe bet for now. Again, thank you for writing up this issue!