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 @resvg/resvg-js to v2.0.0 #606

Merged
merged 2 commits into from May 2, 2022
Merged

Update @resvg/resvg-js to v2.0.0 #606

merged 2 commits into from May 2, 2022

Conversation

XhmikosR
Copy link
Member

It seems Node.js 12 fails with a Segmentation fault

@yisibl do you have any ideas?

@yisibl
Copy link
Contributor

yisibl commented Mar 13, 2022

I run npm test locally and have many test cases failing (node.js 16).
image

@XhmikosR
Copy link
Member Author

Weird, it works for me locally with Node.js 16. The only failure I see is the segmentation fault on Node.js 12 on CI.

@yisibl
Copy link
Contributor

yisibl commented Mar 13, 2022

The main branch (resvg v1.x) doesn't pass even in my local tests. So I can't troubleshoot it now, can you check what recent changes to the test case might have caused it to fail?

@XhmikosR
Copy link
Member Author

@yisibl everything works fine here and on CI with resvg 1.x. This branch here only fails with Node.js 12, and the only change is the resvg update to 2.x. :/

@Kreeg
Copy link
Member

Kreeg commented Mar 13, 2022

@yisibl mind this puppeteer/puppeteer#8009. Maybe you have the same problem

@yisibl
Copy link
Contributor

yisibl commented Mar 13, 2022

mind this puppeteer/puppeteer#8009. Maybe you have the same problem

@Kreeg I've connected the MacBook's power supply and it still doesn't work. Maybe we should look for a more reliable png diff solution.

@yisibl
Copy link
Contributor

yisibl commented Mar 14, 2022

My guess is that this issue(swc-project/swc#2276 (comment)) with Rust may be the cause, @Brooooooklyn is helping to investigate further.

@XhmikosR XhmikosR changed the title Test @resvg/resvg-js 2.0.0-alpha.3 Test @resvg/resvg-js 2.x Mar 14, 2022
@yisibl
Copy link
Contributor

yisibl commented Mar 14, 2022

Please be patient, we are still troubleshooting the cause of the crash, I have 100% reproduced the problem on the Mac, for details of the debugging process please see: yisibl#3

@XhmikosR
Copy link
Member Author

XhmikosR commented Mar 14, 2022

@yisibl I didn't mean to put more pressure on you, sorry about that! I was just stating my plan :)

Thanks for looking into the issue!

@yisibl
Copy link
Contributor

yisibl commented Mar 14, 2022

@XhmikosR It doesn't matter

@Kreeg

This comment was marked as off-topic.

@XhmikosR

This comment was marked as off-topic.

@yisibl
Copy link
Contributor

yisibl commented Mar 18, 2022

@XhmikosR After various debugging, we finally located a possible NAPI bug that was fixed in Node.js 14+, but not cherry-picked to Node.js 12.x.

Please go to support this issue at nodejs/node#42376

@XhmikosR
Copy link
Member Author

@yisibl thanks for digging into it! I notice the Node.js developers have already added the needed labels, let's hope they pick the patch before Node.js 12 goes EOL.

@XhmikosR XhmikosR force-pushed the resvg2 branch 3 times, most recently from a2f372f to c614933 Compare March 24, 2022 19:27
@XhmikosR
Copy link
Member Author

@yisibl I don't suppose it's possible to add width and height to the 1.x branch?

@yisibl
Copy link
Contributor

yisibl commented Mar 26, 2022

@XhmikosR Yes, resvg-js v1.x does not intend to add this API.

@yisibl
Copy link
Contributor

yisibl commented Apr 1, 2022

Good news, now the last version of Node.js 12.x has been released: 12.22.12-rc.0 🚀🚀🚀, PR #42531. The tests have passed!

Download: https://nodejs.org/download/rc/v12.22.12-rc.0/

image

https://github.com/yisibl/svg-sprite/runs/5781947632?check_suite_focus=true

@yisibl
Copy link
Contributor

yisibl commented Apr 1, 2022

You need to change to setup-node-nvm in CI to install Node.js 12.22.12-rc.0, please refer to: yisibl@48a402f

@Kreeg
Copy link
Member

Kreeg commented Apr 1, 2022

@yisibl I just thought the stable release has already been deployed. I think the better way is just to wait for the release

@yisibl
Copy link
Contributor

yisibl commented Apr 6, 2022

Node.js 12.22.12 already released. nodejs/node@a3d2837

@Kreeg Kreeg force-pushed the resvg2 branch 2 times, most recently from d9781f6 to 33a8645 Compare April 6, 2022 12:52
@Kreeg
Copy link
Member

Kreeg commented Apr 6, 2022

Ok! All tests are passed

@XhmikosR
Copy link
Member Author

XhmikosR commented Apr 6, 2022

Let's wait until 12.22.12 has landed in the GitHub images. But this makes me wonder if we should bump the package.json version or not. Probably not, since AFAICT things will work fine unless someone has SVGs without dimensions.

@XhmikosR XhmikosR marked this pull request as ready for review April 23, 2022 06:18
@XhmikosR XhmikosR changed the title Test @resvg/resvg-js 2.x Update @resvg/resvg-js to v2.0.0 Apr 23, 2022
@XhmikosR XhmikosR marked this pull request as draft April 23, 2022 06:21
@XhmikosR
Copy link
Member Author

@yisibl do you plan to release v2.0.0 final soon?

@XhmikosR XhmikosR added dependencies Pull requests that update a dependency file no-squash labels Apr 23, 2022
@yisibl
Copy link
Contributor

yisibl commented Apr 30, 2022

@yisibl do you plan to release v2.0.0 final soon?

resvg-js v2.0.0 final is released. yisibl/resvg-js@ecc7a56

@Kreeg Kreeg marked this pull request as ready for review April 30, 2022 20:32
@Kreeg
Copy link
Member

Kreeg commented Apr 30, 2022

@yisibl that's cool! Thank you!

@Kreeg Kreeg merged commit c02abfa into main May 2, 2022
@Kreeg Kreeg deleted the resvg2 branch May 2, 2022 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file no-squash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants