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

Update the node engine in package.json #1034

Merged
merged 6 commits into from
Apr 20, 2023

Conversation

remcohaszing
Copy link
Contributor

This is a follow up of es-joy/jsdoccomment@3c6b512#r109800324.

@brettz9
Copy link
Collaborator

brettz9 commented Apr 20, 2023

I think if you could bump the minimum to Node 16, it would be best. Node 14 is going to be end-of-lifed April 30th, and in doing so, we can also bump the .babelrc.json targets to Node 16, allowing potentially better performance by avoiding kludges being added to fix a missing feature. The latter change can therefore also be added to the PR.

@remcohaszing
Copy link
Contributor Author

remcohaszing commented Apr 20, 2023

Do you also want me to update to package-lock.json version 3? That was also being blocked by Node.js 14. (Version 2, the current, is literally just both version 1 and 3 combined.)

@remcohaszing
Copy link
Contributor Author

I understand why ESLint is failing, but not how to resolve it. What is .ncurc.js?

@brettz9
Copy link
Collaborator

brettz9 commented Apr 20, 2023

Do you also want me to update to package-lock.json version 3? That was also being blocked by Node.js 14. (Version 2, the current, is literally just both version 1 and 3 combined.)

Yes, that'd be great.

@brettz9
Copy link
Collaborator

brettz9 commented Apr 20, 2023

I understand why ESLint is failing, but not how to resolve it. What is .ncurc.js?

It is the config file for npm-check-updates. It had been preventing updating two packages until Node 16 was the minimum. You don't have to update those packages with this PR (the next time I run npm-check-updates or ncu on the command-line, it will auto-update those packages for me without need for dependabot, etc.)

@brettz9
Copy link
Collaborator

brettz9 commented Apr 20, 2023

so to resolve, you can remove all of these lines:

    // Todo[engine:node@>=16]: Requires Node 16
    'eslint-config-canonical',
    'glob',

@remcohaszing
Copy link
Contributor Author

Already figured it out and removed those. 😄

@brettz9
Copy link
Collaborator

brettz9 commented Apr 20, 2023

I think I'll sit on this until the 30th in case there are updates between now and then. I know other projects transition earlier, but I'd rather avoid any further issues for users esp. with the other breaking changes.

@brettz9
Copy link
Collaborator

brettz9 commented Apr 20, 2023

Btw, how should we get past the fact that this repo is still expecting a Node 14 test?

@remcohaszing
Copy link
Contributor Author

Huh? I don’t understand why that is. Does GitHub not allow external contributors to remove CI jobs? 🤔

@brettz9
Copy link
Collaborator

brettz9 commented Apr 20, 2023

ah, maybe this is something @gajus has to do...

@remcohaszing
Copy link
Contributor Author

Maybe it’s because the original commit still had that job? Maybe it helps if I make a new PR.

@brettz9
Copy link
Collaborator

brettz9 commented Apr 20, 2023

You could try rebasing into a single commit and force-pushing, or sure, if you find that too cumbersome or think it'll be necessary, a new PR...

@remcohaszing
Copy link
Contributor Author

That doesn’t seem to work.

@brettz9
Copy link
Collaborator

brettz9 commented Apr 20, 2023

It rings a bell that Gajus may need to approve.

@gajus gajus merged commit 597e255 into gajus:main Apr 20, 2023
@remcohaszing remcohaszing deleted the update-node-engine branch April 20, 2023 15:48
@github-actions
Copy link

🎉 This PR is included in version 43.0.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants