support for electron 32 #973
Comments
|
In Electron v31 the following lines appeared as warnings In Electron v32 it now throws these errors: |
|
Based on v8/v8@6ec8839 since V8 12.5.59 there is a need to use the |
|
Could this used be to replace |
|
Based on some comments in linked commits, MemoryPressurrNotification seems to correspond more to some GCLimit-ish-named thing not exposed by NAN. I do not see a straight replacement for this, so it will just be deprecated deplaced by a no-op.
…On August 22, 2024 10:42:43 AM GMT+03:00, agracio ***@***.***> wrote:
Could this used be to replace `IdleNotificationDeadline` in a different part of the code?
```
node_modules\nan\nan.h(700,39): warning C4996: 'v8::Isolate::IdleNotificationDeadline' : Use MemoryPressureNotification() to influence the GC schedule.
```
--
Reply to this email directly or view it on GitHub:
#973 (comment)
You are receiving this because you commented.
Message ID: ***@***.***>
|
|
I created an untested PR that should deal with this. Please try it out. |
|
I am getting this error #if defined(V8_MAJOR_VERSION) && (V8_MAJOR_VERSION > 12 || (V8_MAJOR_VERSION == 12 && defined(V8_MINOR_VERSION) && V8_MINOR_VERSION >= 5)
// this part has 3 opening '(' and 2 closing ')'
&& (V8_MAJOR_VERSION > 12 || (V8_MAJOR_VERSION == 12 && defined(V8_MINOR_VERSION) && V8_MINOR_VERSION >= 5)
Same in lines 2619, 2672
|
|
Thanks for reporting, I have updated the PR. |
|
Ok this one is on me, instead of just building your branch of |
|
Alright, so |
|
Apparently AccessControl is already being silently ignored on the other form of |
|
After the fix now getting even more very strange errors. EDIT: If that helps your branch of nan builds 'Test' build and runs |
|
Okay, that one about missing Get was my oversight. I missed that the CallbackInfo object was from NAN and does not have a way of getting the actual value. I tried to do something with a friend function to get around it, but it still might need some massaging before it compiles. I currently do not have anything set up with such a new V8 version so I cannot easily test myself. |
|
Well here is a new list of errors |
|
The friend declaration was not correct. Now it should compile. |
|
Now only have these 2 left, happens on Electron 31 and 32 version 30 runs without those problems. |
|
Great. It is moving in the right direction. I noticed that the signature of property setters had changed further, so I made another update. |
|
Getting closer |
|
That is unfortunate. I cannot think of a reasonable way of taking a |
You mean there isnt a way for nan to support Electron 32+ or anything based on newer versions of v8? |
|
Unfortunately, that is how it appears. It might not be the final answer, but I cannot think of a solution for papering over this change. |
|
I understand that this appears to be an issue that cannot be dealt with easily but should nan users expect any long term plan for Electron 32+ support? |
|
The goal of this project has always been to provide backwards compatibility by providing a consistent API across all versions. I have wondered for the past ten years when a breaking change would come. The cause for this change in V8 is compliance with the ECMAScript standard, changing the underlying semantics. Last time a big break came was whatever V8 version came with Node 4.3, which added the optionals (Maybe, MaybeLocal), but that could still be shoehorned in to the older versions. The difference here is that I do not see a clear way of making the callback signatures work backwards. The internals of V8 are locked down through private access modifiers, making it nigh impossible to do many of the required behind-the-scenes operations when trying to modify its behavior from the outside. Now, supposing that this last issue with the callbacks could be resolved, it would also be necessary to detect if the callback has thrown an exception and account for that when returning the boolean indicating whether the interceptor did anything. There might be more gotchas hiding in the rough. I have not looked into the feasibility of this. |
|
So if I understand it correctly you saying there are no plans to add support for newer v8 versions in any foreseeable future? |
|
Plonk.
This is a common misconception. We don't support electron. If you look at the README electron isn't mentioned anywhere. It's the other way around: Electron promises compatibility with node.js ... and breaks that promise on a regular basis. This is also why, from our perspective, projects like electron, some call them franken-builds, are a bit of a nuisance. Anyway, after a quick glance I agree with @kkoopa. This looks like an upcoming breaking change. NAN 3.0 ... finally... yay?
... and when that happens I'm sure we will take another stab at it. 😁 |
|
Thanks to everyone for the work! |
|
@steinmetz85 I think it was not the case of Electron being an early adopter but rather that Chromium v128 was shipping with v8 12.8, but I might be entirely wrong. It appears that v8 12.8 was added to main Node.js branch a couple of weeks ago so fingers crossed for Node.js v23 nan support or maybe even earlier while v23 pre-release undergoing testing. https://github.com/nodejs/node/blob/main/deps/v8/include/v8-version.h Commit: deps: update V8 to 12.8.374.13 |
|
First of all wanted to say that I do appreciate the work, very quick responses and attempts to fix the error from everyone, in my previous posts I was rather frustrated that the problem is being ignored since Now that v8 12.8 was merged into main Node.js branch 23.0.0 I tested on a different project that does not depend on Electron and unfortunately getting exact same errors. Commit: deps: update V8 to 12.8.374.13 Initially getting the same errors as with Electron build and after applying patch from
Node.js 23 release is a couple months away so while it does not require urgent attention perhaps there should be some plans to address it. |
fixed linkage error for multiple function definitions. refs nodejs#973
|
@toshiyuki-rs 's electron v32 support works :) tested on electron v32.1.2 |
Yes :) oc-soft/npm-nan@4d1d74e...6c6e8aa I install the regular NAN distribution from NPM: https://www.npmjs.com/package/nan/v/2.20.0 ...but I create a patch and apply it via
(in a Docker context without the root Git directory of your main project, simply remove |
|
Tried it but I don't think im running it correctly, can i just run it in root of project that has Opted for this in my package.json |
|
Also works with upcoming Node.js 23 native libraries. |
I removed wrong preprocessor directive. refs nodejs#973
|
@kkoopa Thanks for your work. But there is still something missing:
It might work with node 20.17.0 but not with v8 12.8 and therefor with node 23+ @toshiyuki-rs In your oc-soft repository there is an additional if before the SetAccessor definiton: Is there a reason, why this isn't part of your pull request? Adding this if statement solves the problem for v8 12.8 |
|
I had originally done it like this: 9fa23b7 |
|
I can confirm that this works with electron 32 |
|
Mhmm, unfortunately NAN |
|
So, for now I will continue to use @toshiyuki-rs : oc-soft/npm-nan@4d1d74e...6c6e8aa ...and I see new commits: https://github.com/oc-soft/npm-nan/commits/main/ is this related? https://issues.chromium.org/issues/348660658 UPDATE: ah, this looks related: |
|
Ah, support for Electron v33 is broken, unfortunately: #978 |
With electron 32.0.1 and its underlying v8 version 12.8, there are mulitple errors when building NAN modules:
nan.h(700,39): error C2039: "IdleNotificationDeadline" ist kein Member von "v8::Isolate"
nan.h(2560,8): error C2039: "SetAccessor" ist kein Member von "v8::ObjectTemplate"
nan.h(2654,15): error C2039: "SetAccessor" ist kein Member von "v8::Object"
nan.h(2730,56): error C2440: "": "initializer list" kann nicht in "v8::NamedPropertyHandlerConfiguration" konvertiert werden
nan_scriptorigin.h(19,1): error C2664: "v8::ScriptOrigin::ScriptOrigin(v8::Localv8::Value,int,int,bool,int,v8::Localv8::Value,bool,bool,bool,v8::Localv8::Data)" : Konvertierung von Argument 1 von "v8::Isolate *" in "v8::Localv8::Value" nicht möglich
The text was updated successfully, but these errors were encountered: