wasp-lang / wasp

The fastest way to develop full-stack web apps with React & Node.js.
https://wasp-lang.dev
MIT License
13.9k stars 1.19k forks source link

Should we pin our NPM dependency versions down even tighter? #646

Open shayneczyzewski opened 2 years ago

shayneczyzewski commented 2 years ago

Is your feature request related to a problem? Please describe. Today, an unlucky user hit an error because nodemon had a bad deploy for about 30 minutes. Here is what they saw:

Web app:
Web app: > TodoApp@0.0.0 start
Web app: > react-scripts start
Web app:
Web app:
Server (stderr): /home/h4r1337/wasp/TodoApp/.wasp/out/server/node_modules/nodemon/bin/nodemon.js:15
Server (stderr):   require('update-notifier')({ pkg }).notify();
Server (stderr):   ^
Server (stderr):
Server (stderr): Error [ERR_REQUIRE_ESM]: require() of ES Module /home/h4r1337/wasp/TodoApp/.wasp/out/server/node_modules/update-notifier/index.js from /home/h4r1337/wasp/TodoApp/.wasp/out/server/node_modules/nodemon/bin/nodemon.js not supported.
Server (stderr): Instead change the require of index.js in /home/h4r1337/wasp/TodoApp/.wasp/out/server/node_modules/nodemon/bin/nodemon.js to a dynamic import() which is available in all CommonJS modules.
Server (stderr):     at Object.<anonymous> (/home/h4r1337/wasp/TodoApp/.wasp/out/server/node_modules/nodemon/bin/nodemon.js:15:3) {
Server (stderr):   code: 'ERR_REQUIRE_ESM'
Server (stderr): }
Server (stderr):

I was able to reproduce on a fresh project, but my old project still worked. This led me to believe it was an NPM dependency upgrade issue.

We specify the following version for nodemon: ^2.0.4. Before the update today, 2.0.16 was the latest and was what was working. The broken projects were using 2.0.17 which was released today. Suspecting a bad update, I watched their npmjs page (https://www.npmjs.com/package/nodemon) and saw they followed it up with a 2.0.18 release. That fixed the issue for our user (after a wasp clean).

Describe the solution you'd like Ideally, we should prevent this from happening in some way. Perhaps this means we use exact versions for our dependencies? Although, as @Martinsos noted it would be nice to get patch updates. So this means we are potentially trading Wasp stability (good) for potentially delaying bug/security fixes for our NPM dependencies (bad). We would also need to be diligent about bumping the versions for each release (probably aided by some automation).

Anyways, let's keep this in mind and discuss. Thanks!

FYI @sodic

sodic commented 2 years ago

Yes, this is a tricky one.

I think it makes sense to determine what we do with versions on a case-by-case basis, depending on how reliable/up-to-date they are.

Of course, we could check what other dependency-rich tools did (e.g., react-scripts). It's probably something along these lines.

Martinsos commented 2 years ago

Oh there is another thing here -> lockfiles! So by using lockfiles, we get both -> they get stability, because they know changes will not suddenly change under their feet, while on the other hand, when they explicitly want to update deps, they can, and those will be tested by them and in dev and in CI but since they are locked all is good.

So in any case, we don't want versions to update on their own, even patch numbers, while in production. For that, the only solution is using lockfiles, since they also lock the deps of deps.

Then, even if we do have a bit loser constraints on the top level deps, it is not such a big issue anymore since devs have time to catch any issues coming from those.

shayneczyzewski commented 2 years ago

Yeah, agree we should probably survey other projects @sodic as a research input into this.

In the lockfile case @Martinsos, do you mean we ship the package-lock.json files somehow? Do you think we will hit any OS-specific issues like we did with the Haskell ones?

Martinsos commented 2 years ago

For when Wasp is going to be ready for production, we will certainly need to be doing this, having some support for locking the dependencies, right? I expect we will do that via package-lock.json files. OS specific issues are present also in npm package lock files, but I don't think we can go around it, we will probably have to solve that as a separate issue. I believe yarn solved that problem in some way hm, but I am not sure.

I believe it will come down to having lock files as part of the source, similar like we do now with prisma migrations -> we will generate them in the generated project and then pull them up into the source of the project.

I created an issue for it some time ago here: https://github.com/wasp-lang/wasp/issues/559 .

shayneczyzewski commented 2 years ago

Makes sense, thanks! I will give it more thought too (haven't considered it much up to now tbh). 👍🏻