Fix builds for Electron 29 #966
Conversation
`ObjectTemplate::SetAccessor` signature has changed in the V8 >= 12, and no longer accepts `v8::AccessControl` parameter. The `v8::AccessControl` enum is in fact a single value now so no public users of nan would be broken if we just stop passing it down to V8.
|
cc @kkoopa |
|
Thank you. I'll get this out next week when I am home. |
|
Yay! Thank you so much! |
|
Hey @kkoopa could we please get a release with this fix <3 |
| @@ -2550,7 +2550,9 @@ NAN_DEPRECATED inline void SetAccessor( | |||
| , getter_ | |||
| , setter_ | |||
| , obj | |||
| #if !defined(V8_MAJOR_VERSION) || V8_MAJOR_VERSION < 12 | |||
There was a problem hiding this comment.
Hi! Electron 28 ships with V8 12.0 and it still supports the AccessControl argument. Using nan 2.19.0 will break module compilation for these users. User can workaround by strict versioning to 2.18.0 instead of ^2.18.0. Should the check be based on module version instead, ex: https://github.com/electron/electron/blob/main/patches/nan/api_remove_allcan_read_write.patch ?
There was a problem hiding this comment.
Oh gosh, I think that'd be better indeed. Did V8 make a breaking API change within the same major release? How come 12.0 isn't the same as 12.2?
There was a problem hiding this comment.
Asked about it in the relevant V8 CL https://chromium-review.googlesource.com/c/v8/v8/+/5006387/comment/65b3eecb_f7eb5d8f/
There was a problem hiding this comment.
I think there's some confusion here about V8's versioning -- V8 doesn't use semver, so major vs minor versions don't carry different meaning for API changes. Rather, for historical reasons, it's synchronised to the chromium release cycle, where V8 12.2 is the V8 version that ships with Chrome 122. It's a bit unfortunate but we're stuck with it for now. We do have a rolling stability guarantee that functions will get a deprecation warning for one release before being removed (see https://v8.dev/docs/api), and this was unfortunately accidentally not followed in this case, but that doesn't prevent API changes across multiple "minor" version bumps.
|
I can push out a minor update with a fix quickly, but let us wait and see what the V8 response is.
…On April 16, 2024 3:20:36 PM GMT+03:00, Robo ***@***.***> wrote:
@deepak1556 commented on this pull request.
> @@ -2550,7 +2550,9 @@ NAN_DEPRECATED inline void SetAccessor(
, getter_
, setter_
, obj
+#if !defined(V8_MAJOR_VERSION) || V8_MAJOR_VERSION < 12
Asked about it in the relevant V8 CL https://chromium-review.googlesource.com/c/v8/v8/+/5006387/comment/65b3eecb_f7eb5d8f/
--
Reply to this email directly or view it on GitHub:
#966 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
The maplibre-gl-js install currently fails on arm64 macs when running Node >= 21. The error is: ``` ../../nan/nan.h:2546:8: error: no matching member function for call to 'SetAccessor' tpl->SetAccessor( ``` The context is that `maplibre-gl-js` depends on `node-canvas@2.x` which has a dependency on the `nan` package and `nan` 2.17 had a bug that surfaced with node v21: nodejs/nan#966. Additionally `node-canvas` does not currently provide pre-compiled binaries for Arm64, so a source compile is needed. So, the solution here is to upgrade the `nan` version to the latest release which fixes support with recent node.
The maplibre-gl-js install currently fails on arm64 macs when running Node >= 21. The error is: ``` ../../nan/nan.h:2546:8: error: no matching member function for call to 'SetAccessor' tpl->SetAccessor( ``` The context is that `maplibre-gl-js` depends on `node-canvas@2.x` which has a dependency on the `nan` package and `nan` 2.17 had a bug that surfaced with node v21: nodejs/nan#966. Additionally `node-canvas` does not currently provide pre-compiled binaries for Arm64, so a source compile is needed. So, the solution here is to upgrade the `nan` version to the latest release which fixes support with recent node.
ObjectTemplate::SetAccessorsignature has changed in the V8 >= 12, and no longer acceptsv8::AccessControlparameter. Thev8::AccessControlenum is in fact a single value now so no public users of nan would be broken if we just stop passing it down to V8.