-
Notifications
You must be signed in to change notification settings - Fork 173
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
chore: update dependencies, remove unused ones #100
Conversation
aduh95
commented
Apr 17, 2022
- This PR removes some dependencies that are no longer necessary as long as we're OK with dropping v12.x support (which turns EOL on April 30th).
- To update the Yarn version used to manage Corepack dependencies, this repo is now using Corepack, so you now need Corepack to build Corepack (but imo that's OK now that Corepack is bundled with all supported Node.js versions).
- I've updated all outdated npm dependencies and GitHub actions.
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.
The prepack
script wont work anymore since the file it points to was removed.
We should add an engines
property to package.json
to actually document the supported Node versions.
The Windows tests are quite flaky, since they rely on the network 😕 |
It's probably related to the changes in this PR though, as the flakyness seems to be quite consistent, but I haven't a Windows machine available to test that 😕 |
I've just tested it on a Windows machine, and I'm not able to reproduce the failure (tested with Node.js 16.15.0 and Node.js 18.0.0), so it is probably a fluke indeed. Let's wait for Node.js 14.19.2 to be released next week as it will contain the fix for the Jest issue on Node.js 14.x. |
Isn't |
It looks like the test flakiness doesn't exist on Node.js 14.x, tests are now green :) |
corepack yarn install --immutable | ||
corepack yarn pack |
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.
Why do you call corepack yarn
rather than just yarn
(same in the other places)?
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.
To make it explicit that we want to use the Corepack version of Yarn, in case Yarn v1.x is installed on the runner. Also, if someone is using this file to know what command to use to run the tests, it's the most likely command to succeed.
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.
In that case, perhaps calling corepack enable
(or perhaps sudo corepack enable
) beforehand would be better - from a CLI user experience point of view, I'd prefer if Corepack didn't require any change, to guarantee reduced friction.
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 agree that might be desirable for CLI usage, I'm doubtful it's a valid point in a CI context. The tests already ensures that calling the corepack
binary is optional once corepack enable
has been run, right?
Adding a corepack enable
step in CI seems "wasteful" to me, but if you feel strongly about this, that's fine by me. wdyt?
This reverts commit dd72a68.
Thanks @aduh95! I'll look to make a release soon, I just need to upgrade the checked-in versions |
* chore: update dependencies Continuation of #100 * git add patch file