[WIP] Node bindings #68

paulyoung · 2016-09-18T20:22:59Z

This relates to #33. Just getting things set up using the info from https://nodejs.org/api/addons.html

This creates a Node.js Addon in C++ with a function that gets re-exported by ES6 JavaScript and then tested using Jest.

From https://github.com/github/gitignore

paulyoung · 2016-09-18T20:26:54Z

FWIW, I tried adding flow but it kept failing on the import for the .node file despite having provided a declaration file for it.

jspahrsummers · 2016-09-18T22:22:47Z

Thanks for doing this! I briefly looked into it not long after writing that issue, and got scared off by all the steps involved, so I'm very grateful. 😅

paulyoung · 2016-09-19T15:44:45Z

No problem, I don't blame you!

I actually forgot about my original idea for this which was to create the node bindings as a very thin wrapper (just the .cc files) and put the idiomatic JavaScript in bindings/js.

That way, other AltJS languages like PureScript purescript/purescript#631 (comment) can have bindings without the overhead of the JS layer.

paulyoung · 2016-09-25T22:56:55Z

I've added the beginning of some bindings according to the Node Addons guide. The extent of my C++ knowledge comes from there, so I'd appreciate any feedback on this so far.

paulyoung · 2016-09-25T22:59:22Z

Also, I'm getting a test failure that I don't quite understand so would appreciate another set of eyes on that:

 FAIL  __tests__/Version-test.js
  ✕ Version (9ms)

  ● Version

    expect(received).toBe(expected)

    Expected value to be (using ===):
      "alpha.0"
    Received:
      "dailybuild"

paulyoung · 2016-09-25T23:58:51Z

Additionally I'm unsure how to deal with calling ArbiterFree in the destructor.

This has become redundant given the direction that the node bindings have taken. This could be ressurected should a more modern API be desired (e.g. property getters) but it seems like a lot of overhead at the moment.

kastiglione · 2016-09-26T05:39:52Z

I don't know anything about Node, but maybe I can ask the right questions or be a rubber duck.

I take it you're saying' ~SemanticVersion() can't just call ArbiterFree(_semanticVersion)? If so, why is that?

paulyoung · 2016-09-26T07:12:19Z

@kastiglione when I added #include <arbiter/Types.h> and did what you proposed I got this error:

../src/Version.cpp:35:3: error: no matching function for call to 'ArbiterFree'
  ArbiterFree(_semanticVersion);
  ^~~~~~~~~~~
../../../include/arbiter/Types.h:15:6: note: candidate function not viable: cannot convert argument of incomplete type
      'const ArbiterSemanticVersion *' to 'void *'
void ArbiterFree (void *object);
     ^
1 error generated.

In the methods I defined, I have to do this little dance:

Isolate *isolate = args.GetIsolate();
SemanticVersion *obj = ObjectWrap::Unwrap<SemanticVersion>(args.Holder());
const char *buildMetadata = ArbiterGetBuildMetadata(obj->_semanticVersion);

Notice that I have to do objc->_semanticVersion. I'm not sure how to do this in the destructor since it has no args

paulyoung · 2016-09-26T07:52:15Z

Somehow the build metadata is being returned when calling getPrereleaseVersion(), and I can't figure out why 🙈

jspahrsummers · 2016-09-26T20:58:41Z

bindings/node/src/Version.cpp

+  if (buildMetadata == NULL) {
+    args.GetReturnValue().Set(Null(isolate));
+  } else {
+    args.GetReturnValue().Set(String::NewFromUtf8(isolate, buildMetadata));


Does this definitely copy the string?

Ooo, good call.

The docs say this ¯_(ツ)_/¯

Allocates a new string from UTF-8 data.

paulyoung · 2016-09-27T06:32:03Z

Given the new operator nonsense and the lack of proper support for overloadable constructors in JavaScript I'm going to use the factory pattern described in the addon guide instead.

This will be closer to how the C functions work anyway, and I should be able to use the shared examples I set up to test both.

paulyoung · 2016-09-27T08:47:11Z

I took a stab at that and added createSemanticVersionFromString but I get a segfault only when the string contains build metadata.

I verified this manually in case it was something invalid coming from the arbitrary string instances.

I imagine this is related to the other test failure.

paulyoung · 2016-09-27T08:48:15Z

bindings/node/src/Version.cpp

+  } else {
+    const char *string = *String::Utf8Value(args[0]->ToString());
+    obj = new SemanticVersion(string);
+  }


I don't feel great about this but wasn't sure how else to support multiple constructors.

Seems fine—and very JavaScripty—to me.

In hindsight it sort of makes sense since JavaScript classes only support a single constructor.

paulyoung · 2016-09-27T08:51:09Z

I've got to be overlooking some memory management here, so any help is appreciated 🙇

jspahrsummers · 2016-09-28T19:38:34Z

bindings/node/src/Version.cpp

+  _semanticVersion = ArbiterCreateSemanticVersionFromString(string);
+}
+
+SemanticVersion::~SemanticVersion() {


I think you should use ArbiterFree() here, like @kastiglione suggested, as long as you store the semantic version pointer as non-const (which is correct, since you own it), and explicitly cast here with static_cast<void *>(…).

Non-const was what I was missing when I tried this earlier.

I did this in 8d7960d but didn't need to cast.

@jspahrsummers – do you know if that would be caught at compile time if it's an issue?

It would be caught, yeah. I'm surprised that it's not an issue, since C++ doesn't have implicit casting to void *.

Or maybe it just doesn't have implicit casting from void *…

jspahrsummers · 2016-09-28T19:39:27Z

bindings/node/src/Version.cpp

+void SemanticVersion::Init(Isolate* isolate) {
+  // Prepare constructor template
+  Local<FunctionTemplate> tpl = FunctionTemplate::New(isolate, New);
+  tpl->SetClassName(String::NewFromUtf8(isolate, "SemanticVersion"));


Does this sort of stuff have to be defined in C++, or is it possible to write more of it in JS? (It'd be nice if as much was JS as possible, since that's what users will see.)

Ah, I see—this is referring to SemanticVersion::New, not some magic symbol. In that case, nevermind, since there isn't much which avoids the C++ class.

In general I wish there was a way to do more of this in JS, but I don't think there is.

This is definitely not what I thought I was getting in for in the beginning 😄

Related: I've been going back and forth on whether it's worth providing ES6 classes that have property getters (think computed properties in Swift).

That way things like version.major would work instead of version.getMajorversion(), but I'm not sure if the overhead outweighs the benefit:

we'd need to subclass or create thin wrappers around everything exported from the native bindings, which may introduce more potential for bugs

we may need to transpile to ES5 if wanting to target older versions of Node

That was the motiviation behind 9a46594.

I think it could be a nicer API so will give it another shot once these bugs are worked out.

jspahrsummers · 2016-09-28T19:44:17Z

bindings/node/src/Version.cpp

+void SemanticVersion::Create(const FunctionCallbackInfo<Value>& args) {
+  Isolate *isolate = args.GetIsolate();
+  const unsigned argc = 5;
+  Local<Value> argv[argc] = { args[0], args[1], args[2], args[3], args[4] };


What happens on out-of-bounds here?

I wonder if it just blindly accesses invalid memory (classic C[++] style). That might explain the test failure you see.

What happens on out-of-bounds here?

Nothing right now, I'll address that.

I tried this but it didn't make any difference:

View diff

- Local<Value> argv[argc] = { args[0], args[1], args[2], args[3], args[4] }; + + Local<Value> argv[argc] = { + args[0], + args[1], + args[2], + (args[3]->IsNull() || args[3]->IsUndefined() ? (Local<Value>)Null(isolate) : args[3]), + (args[4]->IsNull() || args[4]->IsUndefined() ? (Local<Value>)Null(isolate) : args[4]) + };

That was an old diff.

View diff

- Local<Value> argv[argc] = { args[0], args[1], args[2], args[3], args[4] }; + + Local<Value> argv[argc] = { + args[0], + args[1], + args[2], + (args.Length() > 3 && (args[3]->IsNull() || args[3]->IsUndefined()) ? (Local<Value>)Null(isolate) : args[3]), + (args.Length() > 4 && (args[4]->IsNull() || args[4]->IsUndefined()) ? (Local<Value>)Null(isolate) : args[4])

That logic is wrong.

- Local<Value> argv[argc] = { args[0], args[1], args[2], args[3], args[4] }; + + Local<Value> argv[argc] = { + args[0], + args[1], + args[2], + (args.Length() <= 3 || args[3]->IsNull() || args[3]->IsUndefined() ? (Local<Value>)Null(isolate) : args[3]), + (args.Length() <= 4 || args[4]->IsNull() || args[4]->IsUndefined() ? (Local<Value>)Null(isolate) : args[4])

paulyoung · 2016-10-12T06:56:42Z

I'm going to take another stab at this soon but welcome any and all ideas in the meantime 😄

paulyoung added 2 commits September 18, 2016 11:24

Add Node .gitignore

2a5de67

From https://github.com/github/gitignore

Initial setup for node bindings

4fbe103

paulyoung added 3 commits September 19, 2016 20:28

Separate Node and JavaScript bindings

1afa9ac

Add make script for building arbiter

cae0af4

Add target for node bindings

a47699f

paulyoung force-pushed the node-bindings branch from 4479725 to a47699f Compare September 22, 2016 04:03

Add bindings for Version, remove demo boilerplate

1c39f36

Fix Version test name

9d39d3e

paulyoung added 2 commits September 25, 2016 16:40

Add linting to node bindings

6ad64b3

Use import syntax since it now works with jest

8e622c1

paulyoung added 3 commits September 25, 2016 17:14

Remove separate JS bindings

9a46594

This has become redundant given the direction that the node bindings have taken. This could be ressurected should a more modern API be desired (e.g. property getters) but it seems like a lot of overhead at the moment.

Prevent coercion from null to string

582cbda

Use JSVerify for testing

b54783e

paulyoung added 2 commits September 25, 2016 23:55

Refactor Version tests

dfe7039

Fix indentation

e958d9c

Account for calls without the new operator

4876284

jspahrsummers reviewed Sep 26, 2016

View reviewed changes

paulyoung added 3 commits September 27, 2016 00:15

Export factory method instead of a class

5d78aaf

Rename NewInstance method to Create

a59c173

Export createSemanticVersionFromString

08af5e1

paulyoung commented Sep 27, 2016

View reviewed changes

Remove unused variable

28ddd59

jspahrsummers reviewed Sep 28, 2016

View reviewed changes

paulyoung added 3 commits September 29, 2016 22:56

Call ArbiterFree in the destructor

8d7960d

Throw errors on incorrect number of arguments

9746499

Update test descriptions

0182d5a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Node bindings #68

[WIP] Node bindings #68

paulyoung commented Sep 18, 2016

paulyoung commented Sep 18, 2016

jspahrsummers commented Sep 18, 2016

paulyoung commented Sep 19, 2016

paulyoung commented Sep 25, 2016

paulyoung commented Sep 25, 2016

paulyoung commented Sep 25, 2016

kastiglione commented Sep 26, 2016 •

edited

Loading

paulyoung commented Sep 26, 2016

paulyoung commented Sep 26, 2016

jspahrsummers Sep 26, 2016

paulyoung Sep 27, 2016

paulyoung Sep 27, 2016

paulyoung commented Sep 27, 2016

paulyoung commented Sep 27, 2016

paulyoung Sep 27, 2016

jspahrsummers Sep 28, 2016

paulyoung Sep 30, 2016

paulyoung commented Sep 27, 2016

jspahrsummers Sep 28, 2016

paulyoung Sep 30, 2016

jspahrsummers Sep 30, 2016

jspahrsummers Sep 30, 2016

jspahrsummers Sep 28, 2016

jspahrsummers Sep 28, 2016

paulyoung Sep 30, 2016

paulyoung Sep 30, 2016

jspahrsummers Sep 28, 2016

jspahrsummers Sep 28, 2016

paulyoung Sep 30, 2016

paulyoung Sep 30, 2016 •

edited

Loading

paulyoung Sep 30, 2016 •

edited

Loading

paulyoung Sep 30, 2016

paulyoung Sep 30, 2016 •

edited

Loading

paulyoung commented Oct 12, 2016

[WIP] Node bindings #68

Are you sure you want to change the base?

[WIP] Node bindings #68

Conversation

paulyoung commented Sep 18, 2016

paulyoung commented Sep 18, 2016

jspahrsummers commented Sep 18, 2016

paulyoung commented Sep 19, 2016

paulyoung commented Sep 25, 2016

paulyoung commented Sep 25, 2016

paulyoung commented Sep 25, 2016

kastiglione commented Sep 26, 2016 • edited Loading

paulyoung commented Sep 26, 2016

paulyoung commented Sep 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulyoung commented Sep 27, 2016

paulyoung commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulyoung commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulyoung Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

paulyoung Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulyoung Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

paulyoung commented Oct 12, 2016

kastiglione commented Sep 26, 2016 •

edited

Loading

paulyoung Sep 30, 2016 •

edited

Loading

paulyoung Sep 30, 2016 •

edited

Loading

paulyoung Sep 30, 2016 •

edited

Loading