Electron 33 broken

[danielweck]

The latest NAN release works fine in Electron 32, but now fails with:

In file included from ../node_modules/nan/nan.h:3045:
../node_modules/nan/nan_scriptorigin.h:40:7: error: no matching constructor for initialization of 'v8::ScriptOrigin'
   40 |       v8::ScriptOrigin(v8::Isolate::GetCurrent()
      |       ^                ~~~~~~~~~~~~~~~~~~~~~~~~~
   41 |                    , name
      |                    ~~~~~~
   42 |                    , To<int32_t>(line).FromMaybe(0)) {}
../node_modules/nan/nan_scriptorigin.h:47:7: error: no matching constructor for initialization of 'v8::ScriptOrigin'
   47 |       v8::ScriptOrigin(v8::Isolate::GetCurrent()
      |       ^                ~~~~~~~~~~~~~~~~~~~~~~~~~
   48 |                    , name
      |                    ~~~~~~
   49 |                    , To<int32_t>(line).FromMaybe(0)
      |                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   50 |                    , To<int32_t>(column).FromMaybe(0)) {}

../node_modules/nan/nan_scriptorigin.h:36:7: error: no matching constructor for initialization of 'v8::ScriptOrigin'
   36 |       v8::ScriptOrigin(v8::Isolate::GetCurrent(), name) {}
      |       ^                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/qwe/Library/Caches/node-gyp/33.0.0/include/node/v8-message.h:64:13: note: candidate constructor not viable: no known conversion from 'Isolate *' to 'Local<Value>' for 1st argument
   64 |   V8_INLINE ScriptOrigin(Local<Value> resource_name,
      |             ^            ~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/qwe/Library/Caches/node-gyp/33.0.0/include/node/v8-message.h:62:17: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
   62 | class V8_EXPORT ScriptOrigin {
      |                 ^~~~~~~~~~~~
/Users/qwe/Library/Caches/node-gyp/33.0.0/include/node/v8-message.h:62:17: note: candidate constructor (the implicit move constructor) not viable: requires 1 argument, but 2 were provided
   62 | class V8_EXPORT ScriptOrigin {
      |                 ^~~~~~~~~~~~

[danielweck]

This follows the fix for Electron v32:

• Support for node version 20.17.0 #976
• replace SetAccessor -> SetNativeDataProperty #977

See: #973 (comment)

[danielweck]

Electron 33 ships with Node 20.18.0:

https://github.com/electron/electron/releases/tag/v33.0.0

[agracio]

Electron 33 moved to v8 13.0 while Node.js main is currently on v8 12.9, perhaps someone can create a working fork that addresses the problem?
Unless by some miracle Node.js moves to v8 13.0 for its v23 release I do not think nan will address the issue themselves.

[danielweck]

Great point @agracio ... pinging @toshiyuki-rs @oc-soft @kkoopa @bugsounet

[agracio]

You should not ping nan maintainers as they have been quick to point out that they do not support and have no intention to support Electron in the future.

[danielweck]

Apologies, I thought you were the NAN maintainer :) 👍 (very quick response indeed, thank you)

Well, my workaround is to comment #include nan_scriptorigin.h in nan.h:

nan/nan.h

Line 3045 in f5921c7

#include "nan_scriptorigin.h" // NOLINT(build/include)

(I do this with sed from my package.json postinstall)

Closing the issue now.

[kkoopa]

It is probably this commit. 0151569 I will get around to it at some future time.

[agracio]

Thanks!

[agracio]

@danielweck could you share your package.json script with postinstall

[kkoopa]

And can someone with Electron 33 confirm that applying 0151569 resolves the issue?

[agracio]

Will do but give me about 30+ minutes.

[danielweck]

@danielweck could you share your package.json script with postinstall

Not pretty, quick and dirty, but works in MacOS and Linux:

"postinstall": "sed 's/^#include .nan_scriptorigin\\.h./\\/\\/ #include nan_scriptorigin.h/' ./node_modules/nan/nan.h > ./node_modules/nan/nan.h.new && mv ./node_modules/nan/nan.h.new ./node_modules/nan/nan.h"

[agracio]

And can someone with Electron 33 confirm that applying 0151569 resolves the issue?

It absolutely does, tested Electron 29, 30, 31, 32 and 33 that are supported by nan 2.22.

[agracio]

@kkoopa So is it just a issue of merging that commit and everything will be working?