-
Notifications
You must be signed in to change notification settings - Fork 8
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
Updating for v5 #40
Updating for v5 #40
Conversation
Apparently, there was a missing peer dependency which then lead to an error. Add a |
.github/workflows/ci.yml
Outdated
@@ -17,6 +17,7 @@ on: | |||
|
|||
jobs: | |||
test: | |||
uses: fastify/workflows/.github/workflows/plugins-ci.yml@v3 | |||
uses: fastify/workflows/.github/workflows/plugins-ci.yml@v4.1.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.
Could you please update to v4.2.1 and pin node versions, it should look sth like:
uses: fastify/workflows/.github/workflows/plugins-ci.yml@v4.2.1
with:
lint: true
license-check: true
node-versions: '["16", "18", "20", "22"]'
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.
Done it
"resolutions": { | ||
"@types/node": "^20.12.7" | ||
}, |
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.
Is this necessary?
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 added that because CI was failing with Yarn: https://github.com/fastify/one-line-logger/actions/runs/8755573058/job/24070941997
I can try to locally re-run the command, and see if something has changed
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.
Just to give you more info, when running yarn
to install deps, we got some warnings:
warning "tap > @tapjs/asserts > tcompare > react-element-to-jsx-string@15.0.0" has unmet peer dependency "react@^0.14.8 || ^15.0.1 || ^16.0.0 || ^17.0.1 || ^18.0.0".
warning "tap > @tapjs/asserts > tcompare > react-element-to-jsx-string@15.0.0" has unmet peer dependency "react-dom@^0.14.8 || ^15.0.1 || ^16.0.0 || ^17.0.1 || ^18.0.0".
warning "tap > @tapjs/test > @isaacs/ts-node-temp-fork-for-pr-2009@10.9.5" has unmet peer dependency "@types/node@*".
Please do note the last one. I dig a little and I can see here (line 153 https://www.npmjs.com/package/@isaacs/ts-node-temp-fork-for-pr-2009?activeTab=code) that @isaacs/ts-node-temp-fork-for-pr-2009
has a peer dependency on @types/node
.
npm
should automatically intall peerDependencies
from v7 (https://github.com/npm/rfcs/blob/main/implemented/0025-install-peer-deps.md), but from what I can gather on Google, it seems yarn
does NOT install those, so..
I'm wondering then why the other two unmet peer dependency does not brake the yarn run test
, but maybe that's because when running that command, we execute code of tap
package that does not rely on react
nor react-dom
Thanks! |
Closes #39
Checklist
npm run test
andnpm run benchmark
and the Code of conduct
CC @simoneb
Just a quick comment: I noticed the folder
benchmark
, but no script to run them. Thus, I wrote these two:Though, I didn't know if we wanted this in the repo, so I didn't push this change