wasp icon indicating copy to clipboard operation
wasp copied to clipboard

Should we pin our NPM dependency versions down even tighter?

Open shayneczyzewski opened this issue 3 years ago • 5 comments

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: > [email protected] 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

shayneczyzewski avatar Jun 23 '22 18:06 shayneczyzewski

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.

sodic avatar Jun 28 '22 08:06 sodic

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.

Martinsos avatar Jun 28 '22 09:06 Martinsos

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?

shayneczyzewski avatar Jun 28 '22 11:06 shayneczyzewski

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 .

Martinsos avatar Jun 28 '22 12:06 Martinsos

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

shayneczyzewski avatar Jun 28 '22 14:06 shayneczyzewski