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 deps to make it work on Node 18 #22

Merged
merged 4 commits into from
Nov 21, 2022
Merged

Update deps to make it work on Node 18 #22

merged 4 commits into from
Nov 21, 2022

Conversation

EHadoux
Copy link
Contributor

@EHadoux EHadoux commented Nov 4, 2022

This PR fixes #20.
node-canvas has been released with pre-built binaries for Node 18, which was the last bit left to make it runnable on all platforms.

Copy link
Contributor

@sternam sternam left a comment

Choose a reason for hiding this comment

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

@dichovsky @EHadoux This looks good to me, just one minor comment is my suggested changes: it is preferable to pin non-dev dependencies to specific versions.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
EHadoux and others added 2 commits November 9, 2022 16:45
Co-authored-by: Allen Merlin Stern <sternam@users.noreply.github.com>
@EHadoux
Copy link
Contributor Author

EHadoux commented Nov 9, 2022

Hey, all done, although I don't think it's preferable at all. Not pinning canvas would have removed the need for this PR. Now we have overrides in package.json so it's fine, but we don't benefit from the tests of this repo.

Copy link
Contributor

@sternam sternam left a comment

Choose a reason for hiding this comment

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

@dichovsky Could you please merge or give write ability to me or @EHadoux ? This is holding up my migration to Node v18

Hey, all done, although I don't think it's preferable at all. Not pinning canvas would have removed the need for this PR. Now we have overrides in package.json so it's fine, but we don't benefit from the tests of this repo.

💯 Thanks @EHadoux! I think it is best practice what you're doing now: to pin to specific version for dependencies but use the ^ in devDependencies.

@dichovsky dichovsky merged commit 2d8fd5f into dichovsky:main Nov 21, 2022
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.

Compatibility with Node v16+
3 participants