-
Notifications
You must be signed in to change notification settings - Fork 54
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
Client package.json should include node version 16.14.2 #112
Comments
Hi, The unfortunate thing about writing a book is that it's set in stone, so when things change to the dependencies the code in the book is built with, breakages can occur, and you then wind up in a situation where if you make changes to the code in the repo, then it doesn't match the book, which becomes just as problematic for readers as the code not working as expected. So, things like this are always a tougher call than they at first seem: do you change the code in the repo and hope the reader can figure out on their own why the code in the book is different? It's a tough call. But, let's think about this... does specifying Node in the dependencies like that even work? If it does, I'm frankly surprised, since you'd need Node and NPM installed to do anything with package.json anyway. So, it seems like a chicken/egg situation. But, it seems like you DID have success with that approach, so I guess Node was installed local to the project and then everything was somehow set up in such a way that it was used instead of the global install? I can't say I'd ever tried that or even knew it was possible, I thought Node had to be installed globally only. So, thanks, you taught ME something :) But, that said, it doesn't seem like the best approach to me, if for no other reason than the kind of confusion I just had. However, it turns out there's an option in package.json that's kind of made for just this kind of situation: the engines option. With it, we could do:
(16.15.1 being the current LTS, and I'm 99.9999% sure the existing code works fine under it). That should then produce a warning or error when you try to install dependencies or run the app via NPM. So, I think that's probably more what we'd want to do here. But, then I come back to the part about the code not matching the book, and since I try to avoid making such changes, I'm thinking even engines is something I wouldn't want to do. But, you DO raise a legitimate issue of course, and I think you also give a good solution: maybe this information should simply be included in the readme. That way, there's no difference between what's in the book and what's in the repo, but now readers will have some indication what the deal is and what to do to get it working. If you agree with that approach, then if you'd like to create a PR for that change, I'd be happy to accept it. Take care, |
Newer versions of node will cause a
loader-runner
error i.e.18.1.0
in the mailbag client:I think the node version should be specified in the
package.json
file in the client under dependencies since there's no README with dependencies listed for the client:16.14.2 is LTS.
Not sure if it's needed, but I ran
npm install [email protected] --save-exact
before putting it inpackage.json
as well.I can do a PR if you want and it checks out.
The text was updated successfully, but these errors were encountered: