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 builds for Electron v31 #1200

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

neoxpert
Copy link
Contributor

Replaced deprecated v8::CopyablePersistentTraits with v8::Global to enabled builds for Electron v31.

@neoxpert neoxpert requested review from JoshuaWise and a team as code owners June 11, 2024 08:58
@neoxpert neoxpert changed the title Support Electron v31 builds Support builds for Electron v31 Jun 11, 2024
Copy link
Member

@mceachen mceachen left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @neoxpert !

There are subtle differences in semantics for v8::Persistent and v8::Global, but I couldn't find a good authoritative explanation other than https://groups.google.com/g/v8-users/c/uSx4F8Uvwis -- but the instance cardinality discussion gave me the willies.

We could minimize impact if this macro was #if/#else ed to only change with the newest version of v8 in electron v31.

@JoshuaWise what do you think?

Copy link
Member

@mceachen mceachen left a comment

Choose a reason for hiding this comment

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

Ah! The deprecation message said to use v8::Global instead of v8::CopyablePersistentTraits -- so let's trust that. I sent @JoshuaWise an email to review this PR (as my approval isn't sufficient to merge it).

@JoshuaWise
Copy link
Member

v8::Global is not copyable; it has move semantics. This change should be fine, as we don't use the copy semantics anymore anyways. However, it's pretty misleading for the template to be called CopyablePersistent. I'll clean this up after merging

@JoshuaWise JoshuaWise merged commit 5ae61c3 into WiseLibs:master Jun 11, 2024
17 checks passed
abhaybuch pushed a commit to felixrieseberg/better-sqlite3 that referenced this pull request Jun 18, 2024
* replace v8::CopyablePersistentTraits with v8::Global

* add Electron v31 to build tasks
abhaybuch pushed a commit to felixrieseberg/better-sqlite3 that referenced this pull request Jun 18, 2024
* replace v8::CopyablePersistentTraits with v8::Global

* add Electron v31 to build tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants