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

Support for Electron 1.4 (V8 5.3.X) #3

Closed
wants to merge 1,403 commits into from

Conversation

bengotow
Copy link
Contributor

@bengotow bengotow commented Oct 4, 2016

This PR replaces the get/setHiddenValue APIs with the new V8 Private APIs. I actually wasn't able to find what version introduced them, and I'm not sure if it's important to continue supporting them when Private is not available. Using the private APIs took a few more lines of code, so it might be nice to create a convenience function if both need to be supported.

I also found that node-bindings was unable to resolve the module's root when running in Electron. This line returns undefined: https://github.com/TooTallNate/node-bindings/blob/master/bindings.js#L55.

This PR should resolve #2!

@JoshuaWise
Copy link
Member

JoshuaWise commented Oct 4, 2016

Thanks for the PR! It is indeed important to support the get/setHiddenValue APIs, since this module needs to continue supporting Node.js 4.x.x and 5.x.x. Some kind of macro will need to get written that detects which version of V8 is being used, and chooses the correct API.

I believe the only way to detect the v8 version is with v8::V8::GetVersion(), which unfortunately returns a const char*, not a number, so some parsing is required. It should be safe to start using the Private API at, and after, version 5.0.71.35.

@bengotow
Copy link
Contributor Author

bengotow commented Oct 7, 2016

Cool sounds good - I'll update the PR!

@JoshuaWise
Copy link
Member

It might be a good idea to rebase this branch with a7a6d65, so the changes from #4 (comment) are included.

@JoshuaWise
Copy link
Member

I've cleaned up and rewritten much of the ancient history that was in better-sqlite3's git repo. You'll need to re-do your changes after cloning a new repo. I'm sorry about the inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Support for Electron?
3 participants