Skip to content

Conversation

@kkoopa
Copy link
Collaborator

@kkoopa kkoopa commented Oct 18, 2024

V8 Was bumped, this would be a patch release, since I already claimed support for Node 23 in 2.22.0.
Also fixes #978.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Oct 19, 2024

Turns out it is not that easy. First, a couple tests need updating. Second, the interceptors are not correctly handled in that they do require return values or will require them soon. I have some workarounds in mind, but implementing them will take a bit of effort. Addons will require a tiny bit of rewriting if using interceptors, but that is par for the course.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Oct 19, 2024

Some tests are still failing related to the Holder change and interceptors. It might be that the tests simply need updating due to the functionality used not being allowed anymore. I do not have time to figure out what the deal is, so this will be stuck until someone clears it up.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Oct 19, 2024

I believe I fixed the tests. Let us wait and see.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Oct 21, 2024

@bnoordhuis Any chance you remember what the deal with Holder versus This in node::ObjectWrap was ten years ago and what it currently is wrt. to, say, the accessors tests here https://github.com/nodejs/nan/blob/main/test/cpp/accessors.cpp? I found this old PR #524, but do not know what applies to which version of what any more.

@devinbinnie
Copy link

+1 this. Ended up needing to manually patch to use Electron v33.

@agracio
Copy link
Contributor

agracio commented Oct 25, 2024

It looks like this PR is stalled, I am using this workaround for my Electron 33 builds: #978 (comment). Works with any version of Electron supported by nan v2.22.0.
Also have an additional workaround for GitHub Action and local scripts for C++ compiler version 20 for both Electron 32 and 33.
Although I only have a C++ v20 compiler failure on Windows but not macOS or Linux runners.

@richardMSFT
Copy link

Any updates on this PR's status?

@cclauss
Copy link
Contributor

cclauss commented Jan 20, 2025

Can the commitment Node.js v23 compatibility be removed from https://www.npmjs.com/package/nan ?

aadcg added a commit to atlas-engineer/cl-electron that referenced this pull request Jan 30, 2025
synchronous-socket depends on nan, which needs to be patched for Electron >= 33.
For the complete discussion and a hotfix see:

nodejs/nan#978 (comment)

Hopefully a PR that fixes it will be merged soon, nodejs/nan#979.

Closes #56.
cclauss added a commit to cclauss/nan that referenced this pull request Feb 7, 2025
@cclauss
Copy link
Contributor

cclauss commented Feb 13, 2025

Please rebase this pull request and edit the following lines to add Node.js v23 to the testing.

matrix: # TODO: Enable 23.x after nodejs/nan#979 or similar.
node-version: [22.x, 21.x, 20.x, 19.x, 18.x, 17.x, 16.x]

@agracio
Copy link
Contributor

agracio commented Feb 28, 2025

#978 appears to be fixed in 2.22.2 presumably in a different PR.
Looking at merged PRs this one stands out as a potential fix: #989 since it is specifically updated v8::ScriptOrigin resolution.

Tested using electron 29-34 as well as 35 beta and latest 36 nightly.

@agracio
Copy link
Contributor

agracio commented Mar 13, 2025

@kkoopa you might want to close this PR since it is no longer relevant and is fixed by nan v2.22.2 see my previous comment #979 (comment)

@cclauss
Copy link
Contributor

cclauss commented Mar 13, 2025

Node.js v23 is not being tested in v2.22.2 https://github.com/nodejs/nan/actions/runs/13548804273 which is worrisome if we claim v23 compatibility.

matrix: # TODO: Enable 23.x after nodejs/nan#979 or similar.

@agracio
Copy link
Contributor

agracio commented Mar 13, 2025

True but I believe this PR was created to address #978 rather than add Node.js test. But I might be completely wrong as a complete outsider to nan.
Since this PR is no longer needed to address #978 there should be another issue+PR opened to focus on adding Node.js 23 to build matrix.
The code changes that were added to address #978 might be a distraction to adding tests for Node.js 23

@cclauss
Copy link
Contributor

cclauss commented Mar 13, 2025

README.md says that Node.js v22 is supported but v23 is not yet listed.

@cclauss cclauss mentioned this pull request May 8, 2025
kkoopa added 6 commits May 13, 2025 04:41
It was removed ages ago, but never marked as deprecated.
Use FunctionCallbackInfo::This instead.

See http://crbug.com/333672197.
This requires minor changes in user code due to the need to call
and return from Intercepted::No or Intercepted::Yes. While an
operator conversion on classes would have been nicer,
implicit conversion to void is not possible.
@nikeee
Copy link

nikeee commented Jun 17, 2025

Node.js 24 is out and there is an issue with NAN:

npm error ../../../nan/nan_callbacks_12_inl.h:112:62: error: ‘const class v8::FunctionCallbackInfo<v8::Value>’ has no member named ‘Holder’
npm error   112 |   inline v8::Local<v8::Object> Holder() const { return info_.Holder(); }

Is this issue related to that or is it a separate one?

@agracio
Copy link
Contributor

agracio commented Jun 17, 2025

PR #1000 merges these changes and addresses issues with Node.js 23 and 24 tests.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jul 10, 2025

Why are the appveyor tests not running now?

@agracio
Copy link
Contributor

agracio commented Jul 10, 2025

I can only guess that you have disabled them, they are running on my branch.
Edit: not sure who you asking tbh.

https://ci.appveyor.com/project/agracio/nan/history

The last build you have is for 2.22.1 so it did not run for 2.22.2 release. If I have to guess it was disabled after all tests were moved to Github Actions.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jul 10, 2025

Webhook log says 403 Unauthorized, somehow something is not getting through there. Last Appveyor build is 5 months old.

@kkoopa kkoopa merged commit 6e22585 into main Jul 10, 2025
30 checks passed
@agracio
Copy link
Contributor

agracio commented Jul 10, 2025

I'm afraid that is not something I can help with.

@cclauss
Copy link
Contributor

cclauss commented Jul 10, 2025

GitHub Actions are the de facto standard CI solution these daze.

@agracio
Copy link
Contributor

agracio commented Jul 10, 2025

Correct but since I removed tests for node.js 8, 10, 12 and 14 from GitHub Actions the idea was that they will run on AppVeyor.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Jul 10, 2025 via email

@cclauss
Copy link
Contributor

cclauss commented Jul 10, 2025

All versions less than v22 are no longer supported.

@agracio
Copy link
Contributor

agracio commented Jul 10, 2025

v20 is Maintenance LTS and is supported but I think we already had this discussion.

It is up to nan maintainers to agree to a support plan.

@cclauss
Copy link
Contributor

cclauss commented Jul 10, 2025

The GitHub organization of this repository is nodejs not nan-maintainers. The url that I provided shows that the v20 EoL date is two weeks ago. If people want to pay for support for EoL versions then work might be done. If not then they will need to replicate bugs in a supported version of Node.js.

@agracio
Copy link
Contributor

agracio commented Jul 10, 2025

From URL that you provided the graph shows approximately April 2026 and then going to https://github.com/nodejs/release#release-schedule v20 EOL is 2026-04-30.
But again I do not think that you are presenting your argument to the right person.

I think you were looking at 'Last Updated' column which shows the date of the last release for that version.

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.

Electron 33 broken

7 participants