Skip to content
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

use GetBackingStore() instead GetContents() #888

Merged
merged 2 commits into from
Mar 6, 2020

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Mar 6, 2020

Use GetBackingStore() instead GetContents() for v8 >= 8.0 as GetContents() has been deprecated. Node.js 14 will use 8.1.

It would be possible to use GetBackingStore() already with 7.9 but this would lead to issues with Node.js 13 as it uses 7.8 till 13.2.0.

Additionally update testmatrix to use latest electron versions and add electron 8.

Refs: nodejs/node#32108
Refs: nodejs/node#30339

Use GetBackingStore() instead GetContents() for v8 >= 8.0 as
GetContents() has been deprecated. Node.js 14 will use 8.1.

It would be possible to use GetBackingStore() already with 7.9
but this would lead to issues with Node.js 13 as it uses 7.8
till 13.2.0.

Additionally update testmatrix to use latest electron versions
and add electron 8.
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % style nit. Thanks, Gerhard!

nan_typedarray_contents.h Outdated Show resolved Hide resolved
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
@kkoopa kkoopa merged commit 2c023bd into nodejs:master Mar 6, 2020
@Flarna Flarna deleted the update-electron branch March 6, 2020 15:14
trinitronx added a commit to trinitronx/node-qrusage that referenced this pull request Jun 11, 2024
 - check for V8_MAJOR_VERSION >= 8, and use new GetBackingStore() API if so

 - See nodejs/nan#888 for more details

 - Relax dependency on nan ~2.14 so this can build on modern Node.js versions
   with nan >= 2.14.1
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.

3 participants