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 #523

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

Conversation

inigomarquinez
Copy link
Member

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 (20, 21 and 22) to the matrix.

Related to jshttp/http-errors#108

@inigomarquinez
Copy link
Member Author

It seems that the tests fail for node >= 21, but not related to this PR itself, but to node itself.

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.

LGTM!

@wesleytodd
Copy link
Member

Ah, I am surprised we have more tests in here failing because of the query thing, but that is unrelated here. @jonchurch did you end up getting this fixed in that other branch? If so, mind dropping a link here so we don't have to repeat work to resolve it.

@wesleytodd
Copy link
Member

I just tested locally and latest 22 does not fail. Do you want to update this to use 22.2.0? I am actually unsure what benefit we get from not just taking the latest in the major line, could we maybe just drop the minor specifier as well?

@wesleytodd wesleytodd mentioned this pull request May 17, 2024
@inigomarquinez
Copy link
Member Author

@wesleytodd , minor version removed from node 22. It still fails for node 21. Should I also removed minor version?

@wesleytodd
Copy link
Member

Yeah I think we should remove the minor from all of them. We really want to be testing the latest in each major line without having to update all of them all the time.

@inigomarquinez
Copy link
Member Author

@wesleytodd , I've removed the minor versions

@UlisesGascon , @carpasse , perhaps something to have in mind for all the other ci pipelines of the repositories we've been fixing/migrating 🤔

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