search

pmed / v8pp

Bind C++ functions and classes into V8 JavaScript engine
http://pmed.github.io/v8pp/
Other
887 stars 119 forks source link

Support for members with accessors in class_ #130

Open yonatan-mitmit opened 4 years ago

yonatan-mitmit commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

pmed commented 4 years ago

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 commented 4 years ago

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

Thanks!

pmed commented 3 years ago

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