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

ci: fix errors in ci github action for node 8 and 9 #48

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

inigomarquinez
Copy link
Contributor

@inigomarquinez inigomarquinez commented Apr 24, 2024

This PR fixes nyc version to 14.1.1 when running tests in node 8 or node 9. nyc 15.x requires a yargs package version that requires node >=10.

I've also added the latest versions of node (18, 19 and 20) to the matrix.

Related to jshttp/http-errors#108

Warning

The CI is failing for one test in node@21 and node@22. The error has nothing to do with this PR, because those node versions were not tested before and with this PR we discovered it.

Copy link
Member

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

Can we include Node@21?

Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

Yeah agree we should add node 21, but everything else looks good!

@inigomarquinez
Copy link
Contributor Author

inigomarquinez commented Apr 30, 2024

Added support for node@21 here

@inigomarquinez
Copy link
Contributor Author

The CI is failing for one test in node@21 and node@22. The error has nothing to do with this PR, because those node versions were not tested before and with this PR we discovered it.

@wesleytodd
Copy link
Member

Interesting, I don't have time to look into the failure right now, but the test is pretty simple and clear so likely this is a change in node as I don't think this package ever returns a 400, so likely we either need to update/remove this test.

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.

None yet

3 participants