Skip to content
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

Merged
merged 5 commits into from
May 4, 2024
Merged

Updating for v5 #40

merged 5 commits into from
May 4, 2024

Conversation

Gesma94
Copy link

@Gesma94 Gesma94 commented Apr 19, 2024

Closes #39

Checklist

CC @simoneb

Just a quick comment: I noticed the folder benchmark, but no script to run them. Thus, I wrote these two:

"bench:format-date": "node benchmark/format-date.js",
"bench:format-message": "node benchmark/format-message.js"

Though, I didn't know if we wanted this in the repo, so I didn't push this change

@Gesma94
Copy link
Author

Gesma94 commented Apr 22, 2024

Apparently, there was a missing peer dependency which then lead to an error. Add a resolutions entry to package.json that should fix the yarn commands

@@ -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
Copy link
Member

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"]'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done it

Comment on lines +24 to +26
"resolutions": {
"@types/node": "^20.12.7"
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary?

Copy link
Author

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

Copy link
Author

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

@gurgunday gurgunday merged commit b44c0c2 into fastify:next May 4, 2024
22 checks passed
@gurgunday
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants