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 members with accessors in class_ #130

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

yonatan-mitmit
Copy link

The PR contains the following changes:

  1. Support for const members pointers (as an overload of set_const). This requires special handling as the setter cannot compile and the readonly runtime check wasn't sufficient
  2. Support for members with special accessors (a class containing a property which has itself special setter/getter). The more natural behavior is that X.y = z will invoke X.y.setZ(). Previous syntax forced using (n JS X.y.z or X.y.setZ() )
  3. Support for "chainable setters" these return T& instead of void and support a "fluent" syntax X.setY().setZ(). In JS they are treated as properties and thus are not chainable, but it doesn't require library changes to get wrapped.

P.S.
Thanks for the awesome library

@yonatan-mitmit
Copy link
Author

The tests above fail on old versions of V8. (I'm testing against 7.8.279.23)
Without these changes, the code doesn't compile on latest.
What's the version you want supported?

@pmed
Copy link
Owner

pmed commented Dec 13, 2019

Hi @yonatan-mitmit

Thank you very much for the contribution! I will review and merge it.

I would prefer to stay with the elder V8 version, as possible. Currently Travis CI runs tests for different V8 versions starting from 6.3, in development I use 7.5.

It seems that V8 has some changes in its API, so the movement to the newer V8 is inevitable. In this case a minimal V8 version should be a reasonable value, e.g. the same as used in Node.js LTS version.

Best regards
Pavel

@yonatan-mitmit
Copy link
Author

Added ifdefs for the V8 version. NewFromUtf8 that returns a Local (And not MaybeLocal) was deprecated in 7.6

@pmed
Copy link
Owner

pmed commented Dec 15, 2019

Hi @yonatan-mitmit,

thanks for the pointing out the issue with v8::String::NewFromUtf8 in v8pp::throw_ex(). It turned out that such an overloaded function returning MaybeLocal exists for a long time.

I've fixed this in context of issue #131, it works with V8 version 7.9. I need to add the newer version to Travis build matrix.

@cr-mitmit
Copy link
Contributor

Thanks the fix.
Can you see re the rest of the PR (Support member's access operator as properties?)

Thanks!

@pmed
Copy link
Owner

pmed commented Jul 31, 2020

Hi @yonatan-mitmit

Can you see re the rest of the PR (Support member's access operator as properties?)

Sorry for the review delay, I had no spare time. The proposed changes look good 👍

Could you please also add a test case, in order to demonstrate how such a property would be bind in C++ and used in JavaScript?

Best regards,
Pavel

@pmed pmed force-pushed the master branch 2 times, most recently from 14bc459 to 352c061 Compare March 14, 2021 14:46
@pmed pmed force-pushed the master branch 2 times, most recently from bd106c3 to 6611145 Compare November 2, 2021 22:44
@pmed pmed force-pushed the master branch 3 times, most recently from 4119f8e to 211ef9b Compare March 19, 2022 19:04
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