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

Add support for electron v29 prebuilds #1147

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

m4heshd
Copy link
Contributor

@m4heshd m4heshd commented Feb 20, 2024

Stable electron v29 just released. Prebuilds were tested here but the tests are failing with the following error.

D:\better-sqlite3\build\src\util\macros.lzz(150,33): error C2664: 'void v8::ObjectTemplate::SetAccessor(v8::Local<v8::String>,v8::AccessorGetterCallback,v8::AccessorSetterCallback,
v8::Local<v8::Value>,v8::PropertyAttribute,v8::SideEffectType,v8::SideEffectType)': cannot convert argument 5 from 'v8::AccessControl' to 'v8::PropertyAttribute' [D:\better-sqlite3\build\better_sqlite3.vcxproj]

Pinging @JoshuaWise.

@m4heshd m4heshd requested review from JoshuaWise and a team as code owners February 20, 2024 06:21
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.

🍻

@mceachen mceachen merged commit d24234b into WiseLibs:master Feb 21, 2024
14 checks passed
@m4heshd
Copy link
Contributor Author

m4heshd commented Feb 21, 2024

https://github.com/WiseLibs/better-sqlite3/actions/runs/7995300838/job/21835431780#step:8:4004

Yup. The same error I mentioned in the note. Needs some work it seems. I wonder if it's time to switch to N-API.

@mceachen
Copy link
Member

(I totally missed your comment -- I just looked at the diff and LGTM'ed it in)

@mceachen
Copy link
Member

@JoshuaWise #1127 seems like it may solve this if those default params are indeed optional.

@m4heshd
Copy link
Contributor Author

m4heshd commented Feb 21, 2024

(I totally missed your comment -- I just looked at the diff and LGTM'ed it in)

Oops. That was my guess when you did the release.

@mceachen
Copy link
Member

mceachen commented Feb 21, 2024

@m4heshd please submit a PR that reverts this, and I'll do another patch-release to roll back.

@JoshuaWise alternatively, you can just roll back the last release: npm dist-tag add [email protected] latest

Sorry I didn't catch your comment!

@m4heshd
Copy link
Contributor Author

m4heshd commented Feb 22, 2024

Sorry I didn't catch your comment!

No worries. Sent the PR.

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.

2 participants