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

fix: yarn package manager is missing getPeerDependencies #1419 #1420

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

rbnayax
Copy link
Contributor

@rbnayax rbnayax commented Jun 1, 2024

fix: #1419

@rbnayax
Copy link
Contributor Author

rbnayax commented Jun 1, 2024

@raineorshine not sure why the test failed, it works locally on my ubuntu

@raineorshine
Copy link
Owner

I re-ran the Node v18 ubuntu test twice, and unfortunately it continues to fail in GitHub Actions.

It looks like the doctor test is passing, but the success message gets written to stderr instead of stdout for some reason. I have seen this behavior occur with bun before, but not with npm. It seems to occur consistently in this branch and not in main.

@rbnayax
Copy link
Contributor Author

rbnayax commented Jun 2, 2024

@raineorshine I fixed the unstable test. I think it has something to do with new npm behaviour on node 18 where it prints the commands it is about to run to stderr.

Copy link
Owner

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

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

We can't completely skip the assertion, as it creates a hole in the test coverage if a future change breaks doctor mode on node v18. We have to assert that stderr is either empty, or just contains the name of the npm script.

I re-ran the tests on main and they failed at the same commit where they succeeded two weeks ago, so it seems that a minor or patch upgrade on node v18 or its bundled npm caused the break. Since this was not related to your PR, feel free to go back to the original one line change you made and ignore the errors. Or you can rewrite the assertion as described above, though that should go in a separate PR.

@rbnayax
Copy link
Contributor Author

rbnayax commented Jun 8, 2024

We can't completely skip the assertion, as it creates a hole in the test coverage if a future change breaks doctor mode on node v18. We have to assert that stderr is either empty, or just contains the name of the npm script.

I re-ran the tests on main and they failed at the same commit where they succeeded two weeks ago, so it seems that a minor or patch upgrade on node v18 or its bundled npm caused the break. Since this was not related to your PR, feel free to go back to the original one line change you made and ignore the errors. Or you can rewrite the assertion as described above, though that should go in a separate PR.

Done. separate PR in #1423

@raineorshine raineorshine merged commit f34082d into raineorshine:main Jun 12, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

yarn package manager is missing getPeerDependencies
2 participants