-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
BREAKING: drop support for Node.js v16; add support for ^20,>=22 #245
Conversation
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@types/node@17.0.23 |
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.
Seems like we may not be able to do this yet until we bump the ESLint packages, or at least eslint-plugin-jsdoc
.
.github/workflows/build-test.yml
Outdated
@@ -29,7 +29,8 @@ jobs: | |||
with: | |||
path: ${{ steps.yarn-cache-dir.outputs.YARN_CACHE_DIR }} | |||
key: yarn-cache-${{ runner.os }}-${{ steps.yarn-version.outputs.YARN_VERSION }}-${{ hashFiles('yarn.lock') }} | |||
- run: yarn --frozen-lockfile | |||
# TODO: Remove ignore-engines when eslint-plugin-jsdoc has been upgraded |
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.
Hmm, it seems that we may want to bump this package first before merging this PR. The fact that we're seeing this error means we would also see this error in development too.
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 only in development/CI, though, and does not affect users of the package.
Think we can address upgrading the lint plugin as a follow-up, in order to unblock #241?
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.
(FWIW, --ignore-engines
matches default behavior in yarn v3+)
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.
Alright, but in the meantime, when I go to this branch and run yarn install
, I see an error because of this package. It seems that not being able to install dependencies would be confusing for people who want to work on this library, no?
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.
Good call, I just updated project .yarnrc
so this will ignore engines by default now (which also means this change in ci is not needed anymore and therefore reverted)
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.
Okay, cool. FWIW I think it's silly that Yarn ignores node.engines
, but I think we've had that discussion before and I guess we can use https://github.com/devoto13/yarn-plugin-engines if we really feel like it. I'm good with your change.
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
https://github.com/DefinitelyTyped/DefinitelyTyped?tab=readme-ov-file#how-do-definitely-typed-package-versions-relate-to-versions-of-the-corresponding-library Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
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.
Blocking