-
Notifications
You must be signed in to change notification settings - Fork 1
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
Chores: Update node version to 20.8.9 #29
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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 install
when 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Please Change]: The whitespace changes in these files should not be part of this PR (see general comment on git history).
91a957c
to
683993c
Compare
c68e302
to
7e9308e
Compare
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.
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.