Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix Docker and upgrade Node #415
Fix Docker and upgrade Node #415
Changes from 12 commits
3c64705
7e66225
3bf256a
a2b6ca2
5afbab2
b28e273
53a4f3e
ebd1be5
dc866d2
596ebdd
1aa51c0
0e6ee65
2b1c742
ab76abb
0924268
08f986c
0634f9d
d8498ea
7fcee93
565a4c3
d40a7cc
9a400b8
4aea59e
ef25374
8ce35b8
8687fd7
d93e3a5
d74bd4d
27e15ca
f83e2f9
a2dd673
211eafc
d2841b3
daa6589
bf85545
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed for Yarn classic (v1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Corepack now, not yarnPath, so this file should be deleted
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the env descriptors, this will default to
22.x
so let's update hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be:
I think I've also seen in the wild:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the one I was suggesting https://hub.docker.com/layers/library/node/22-alpine3.20/images/sha256-85235be9f710c1ca817cb67892544b7f0cf994ef05380e8ac0d2ea58dce8a988?context=explore
I messed up when adding the suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the --immutable flag here as a precaution. The Docker build will fail if the Yarn install process would result in a different yarn.lock file, which could happen for example if two different versions of Yarn are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command doesn't actually exist anymore but it was never updated. Probably because the image is never run with this default command but rather a command override is supplied by Docker Compose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why splitting commands then explanations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you can copy and paste the commands all at once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't actually pass these options this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the real source of the "node_modules state file not found" error, not the Yarn version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are really the only folders that can be watched for hot-reloading. Mounting everything was causing the .env file (which gets copied in the build step, see the Dockerfile) to go missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but why? package.json has
>=18.0
so this is our canonical version. So let's just bump all to>=22.0<23.0
explicitly to avoid mismatchSee https://endoflife.date/nodejs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about doing that in a follow-up PR, hence:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, just saw your comment on that issue (#416). Since I'm already neck-deep in all of this, I'll go ahead and update Node