-
Notifications
You must be signed in to change notification settings - Fork 76
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
Node v20.5.0 issue and fix #43
Comments
Faced this problem as well when trying to use it with Node 20, with the above fix it did indeed work, but still I am sticking to Node 18 for now until this is maybe integrated. @zack09holland I think your solution would be good PR material :) @missinglink would this be accepted and a new package version published for Node 20 compatibility? |
I would happily accept any patch which improves the supported versions. Since it's a pain to test manually, it would be best if the code was accompanied by a CI script which tests it on every release. |
I can certainly write up some CI tests to go along with my PR. Mostly wanted to document my findings in case someone else was encountering the same issue. |
Please test the code in #44 and let me know if this issue is resolved. |
@missinglink I had created this issue just to document my findings. Changes in #44 is the pull request I opened up as well. This issue can be closed. |
I was getting this error originally
When on Node v14.18.0, you have to do a node-gyp rebuild within the node-postal module folder, which builds successfully and will recreate the bindings. However when I would do this on Node v20.5.0 it was failing during the make stage
Spent a few hours researching both of those issues. I went through the debug logs and saw an error
On an entirely different packages github issues post, someone commented with a change to get the correct JS context and sure enough I was able to run
node-gyp rebuild
within the node-postal directory to rebind it and the errors went awayWithin parser.cc and expand.cc change:
v8::Local<v8::Context> context = exports->CreationContext();
to
v8::Local<v8::Context> context = exports->GetCreationContext().ToLocalChecked();
The text was updated successfully, but these errors were encountered: