Chores: Update node version to 20.8.9 #29
Conversation
There was a problem hiding this comment.
A few requested changes (pardon the delay). Also, noticing that git history is a bit weird here - it looks like this branch was created off of an old feature branch and that's causing the old commits to show up here (along with whitespace changes probably).
Let's sync on this later to run through the rebase process to fix that; we can also knock out the other changes here. Thanks for jumping on this!
.nprmc
Outdated
| @@ -0,0 +1,2 @@ | |||
| # .npmrc | |||
There was a problem hiding this comment.
[Nit/Please Change]: This comment should be removed, it's just a comment to indicate the file copied from StackOverflow.
.nprmc
Outdated
| @@ -0,0 +1,2 @@ | |||
| # .npmrc | |||
| engine-strict=true | |||
There was a problem hiding this comment.
[Please Change]: Noticing that this doesn't actually prevent npm install locally. From reading about this it seems like there's a lot of quirks with this setting in NPM, and our objective of specifying engines is more for documentation so there's no real harm if this isn't there. Either way, one of two options:
- This should properly prevent
npm installwhen on incompatible versions (e.g. Node 18) - This should be removed as it doesn't work (preferred for simplicity).
[Nit]: Newline at end of file.
There was a problem hiding this comment.
Sorry I did not get to this sooner. Since both lines of the .nprmc file are removed through this and the previous conversation, that leaves .nprmc completely empty. Should I just remove the .nprmc file since there's nothing in it, leave it as is, or should I make some sort of change to it that keeps some kind of functionality?
There was a problem hiding this comment.
Correct, we don't need this file. It would be great if some of this stuff worked as advertised but NPM tends to fall short on a lot of that.
package.json
Outdated
| @@ -31,5 +31,8 @@ | |||
| "postcss": "^8.4.31", | |||
| "tailwindcss": "^3.3.5", | |||
| "typescript": "^5.2.2" | |||
| }, | |||
| "engines" : { | |||
There was a problem hiding this comment.
[Nit/Please Change]: engines should go near the top as it's a pretty important setting. I would suggest before scripts, since scripts is the first thing that will start running things using Node.
package.json
Outdated
| @@ -31,5 +31,8 @@ | |||
| "postcss": "^8.4.31", | |||
| "tailwindcss": "^3.3.5", | |||
| "typescript": "^5.2.2" | |||
| }, | |||
| "engines" : { | |||
| "node" : ">=19.0.0 <=20.8.9" | |||
There was a problem hiding this comment.
[Please Change]: This is incorrect per SemVer - with SemVer specifications, we should specify the lower bound as the minimum version supported. Since we're using Node 20, this should be at least 20.0.0. It's also possible to specify higher versions that way we don't test on (say) 20.8.9 successfully but earlier versions might be missing features or have bugs. Two options:
>= 20.0.0., aka any version of Node 20 (Recommended: this is the closest match to our workflow files right now which use20.x).>= 20.8.9, aka at least the current version of Node which has been tested. This is also fine, but matching our workflows makes more sense.
src/app/globals.css
Outdated
| to bottom right, | ||
| var(--background-gradiant-dark), | ||
| var(--background-gradiant-light) | ||
| to bottom right, |
There was a problem hiding this comment.
[Please Change]: The whitespace changes in these files should not be part of this PR (see general comment on git history).
WillBAnders
force-pushed
the
chores/update-node
branch
from
91a957c
to
683993c
Compare
WillBAnders
force-pushed
the
chores/update-node
branch
from
c68e302
to
7e9308e
Compare
There was a problem hiding this comment.
All good now. As mention on Discord, rebase removes merge commits by default so that's why those weren't showing up right. Still unsure why the changes from the merge were showing up in the diff, but oh well.
Also added a commit to use ^ instead of >=; same effect but matches our other dependency versions for consistency.
Double-check the changes here are correct and merge when ready!
Updated node version to 20.8.9, made it so lint and test would both use this higher version, created .nprmc to enforce the use of this higher version. Simple dependency bump.