-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat!: establish baseline Node.js version support #184
Conversation
@nzakas Given this package is not doing anything especially fancy, would you consider dropping macOS from the build matrix? I'm not sure it's buying us much. |
No objections from me. |
c04dd0e
to
1052075
Compare
@nzakas friendly ping for approval |
Gah sorry, just been buried lately. Thanks for the followup ping. |
@@ -70,5 +70,8 @@ | |||
"rollup": "4.23.0", | |||
"typescript": "5.6.3", | |||
"yorkie": "2.0.0" | |||
}, | |||
"engines": { | |||
"node": "^16.20.0 || ^18.0.0 || ^20.0.0 || ^22.0.0" |
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 generally consider changing explicitly supported node versions as a breaking change. I don't mind doing it, but I do think we should bump major version for clarity. WDYT?
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.
Yes, that's fine. You do the releases/bumps manually, 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.
It's handled via release-please, so if we update the title of this PR to be "feat!: Set Node.js support"
(or some such thing with "feat!" at the front), when this is merged, release-please creates a major version bump.
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.
great, right. I probably should have checked 😄
BREAKING CHANGE: Adds `engines.node` field in `package.json` with value of `^16.20.0 || ^18.0.0 || ^20.0.0 || ^22.0.0`. Versions which do not satisfy this range are unsupported. - Adds Node.js v22 to build matrix
1052075
to
41b3c07
Compare
@nzakas OK, this is ready for final approval. |
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.
LGTM.
I wonder if adding the
engines
field is going to break somebody. I guess I'd only really care if their package manager's default behavior is like enablingengineStrict
? Is that a thing?