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

Could someone take a look at updating node-canvas N-API implementation that Jason did back in 2017? #1250

Closed
GitMurf opened this issue Dec 15, 2022 · 6 comments
Labels

Comments

@GitMurf
Copy link

GitMurf commented Dec 15, 2022

@jasongin did a wonderful job at porting node-canvas from NaN to N-API via node-addon-api back in 2017 but there has not been an update since and unfortunately node-canvas has some key things they have changed since then that makes the old fork unusable with any libraries that depend on node-canvas today.

See details here: Automattic/node-canvas#923 (comment)
Here is that fork of his from 2017: https://github.com/jasongin/node-canvas/tree/napi

I have spent weeks trying to figure this out but it is over my head. I started from the NaN to N-Api conversion tool but I can't for the life of me figure out how to do the rest of the cleanup / conversion to make the newest version of node-canvas work (I have zero C/C++ experience so it is a bit foreign to me).

Is anyone willing to take a look to see if you can port the current version of node-canvas to N-Api? Or even if you just port over a couple of the cc/h Class files then maybe that could be enough for me to use as a template to get the rest of it converted.

Thanks in advance!

@GitMurf
Copy link
Author

GitMurf commented Dec 26, 2022

I also believe updating this library would be a great case study for updating the conversion.js script as there are a lot of repeated patterns in this NaN to Napi library conversion (after running the conversion.js script) that I have had to apply as I have tried to figure the conversion out on my own.

In other words, I think the conversion.js script could be greatly enhanced by taking some of the learnings of the extra conversions that must be done with this node-canvas library after running the initial conversion.js script.

Let me know if you have any questions... thanks!

@NickNaso
Copy link
Member

@GitMurf sorry if I'm answering only now, but you're tright it could be interesting take a look at node-canvas. Now I'm in vacation and I will try to work on this starting from 3rd January 2023.

@GitMurf
Copy link
Author

GitMurf commented Mar 21, 2023

Hi @NickNaso … hope 2023 has been treating you well! Checking in to see if you’ve considered taking a look at node canvas? :) thanks in advance!

@GitMurf
Copy link
Author

GitMurf commented Apr 24, 2023

Looks like @chearon finished the port of node-canvas to N-API and just opened a PR for it. Would be wonderful if @NickNaso or someone from the node addon team could take a look and provide any comments or feedback! Thanks a bunch in advance! Here is the PR: Automattic/node-canvas#2235

@github-actions
Copy link
Contributor

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

@github-actions github-actions bot added the stale label Jul 24, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 24, 2023
@GitMurf
Copy link
Author

GitMurf commented Sep 27, 2023

This has been completed! Nice work by everyone involved! 🎉 Especially @chearon with the help of @zbjornson and @gabrielschulhof!

See merged PR here: Automattic/node-canvas#2235

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

No branches or pull requests

2 participants