The official "addressbook" example working in JS #56

efokschaner · 2017-07-19T10:33:44Z

This introduces capnpc-js, and a new js-examples package that should be prominent within the repo, and have minimal dependencies to serve as a reference. It doubles as a full integration test of the system (although it's a little light on validation, as is the original in the C++ repo).

efokschaner · 2017-07-19T10:34:15Z

.travis.yml

@@ -12,13 +12,14 @@ before_install:
  - export PATH="$HOME/.yarn/bin:$PATH"
 install:
  - yarn --frozen-lockfile
-  - yarn run bootstrap -- --frozen-lockfile


I made bootstrapping a "post install" of the monorepo.

Great idea. 👍

efokschaner · 2017-07-19T10:35:12Z

.vscode/settings.json

+    "**/node_modules": true,
+    "**/bower_components": true,
+    "**/lib": true,
+    "**/lib-test": true


Let me know if you can't bear these changes. Personally i find keeping the generated code out of the search results is more important that keeping it out of the IDE, I like to poke around in it occasionally.

That shouldn't bother me. It's also nice for tracing unmapped stack traces (tsc's output is super nice and legible, unlike babel...).

efokschaner · 2017-07-19T10:36:45Z

README.md

-npm install -g capnpc-ts
+npm install -g capnpc-ts # For TypeScript
+# OR
+npm install -g capnpc-js # For JavaScript


Have you registered these npm names, or whatever the process is?

As you'll want to register capnpc-js too

"Registering" is simply a matter of publishing first.

Suppose I'll just do that today with zero fanfare so nobody tries using it yet...

efokschaner · 2017-07-19T10:40:10Z

package.json

@@ -5,11 +5,8 @@
  },
  "description": "Strongly typed Cap'n Proto implementation for the browser and Node.js using TypeScript",
  "devDependencies": {
-    "@types/benchmark": "1.0.30",


these don't need to be added here, lerna bootstrap --hoist effectively automatically takes care of this for you.

efokschaner · 2017-07-19T10:40:43Z

package.json

    "codecov": "^2.2.0",
+    "glob": "^7.1.2",


you added this to dependencies but I think its just a devDependency, but let me know if i was mistaken.

Correct. Good catch.

efokschaner · 2017-07-19T10:41:22Z

package.json

    "benchmark": "gulp benchmark",
    "build": "gulp build",
-    "ci": "gulp lint && gulp coverage && codecov --disable=gcov",
+    "ci": "gulp lint && gulp coverage && codecov --disable=gcov && cd js-examples && ./test.sh",


We should probably have a gulp ci target or something?

Yes. I'm also a bit reticent about having test.sh be UNIX only, since that means you need a *NIX box to work on this library.

Sigh maybe that's fine.

The upstream source does it. We want simplicity in this package as it serves as the basic usage example. But I'm happy to add a .bat for Windows perhaps?

I mean it's vital for the library itself to be cross-platform (and it very much is).

For development... eh. If people want to actually do dev on Windows I'll accept a patch but this isn't really worth fussing about now.

Us windows devs are pretty used to porting those npm scripts to batch to get things working.

Seriously however I don't see much issue with having a small shell script, a small batch script and declaring ourselves fully cross platform.

efokschaner · 2017-07-19T10:42:14Z

package.json

    "coverage": "gulp coverage",
+    "lerna": "lerna",


nifty to have npm run lerna -- ... but let me now if you'd prefer not to have it.

Doesn't hurt. Something like npx seems like the real answer.

Nifty tool, thanks.

codecov · 2017-07-19T10:45:15Z

Codecov Report

Merging #56 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #56   +/-   ##
=======================================
  Coverage   75.47%   75.47%           
=======================================
  Files          48       48           
  Lines        2924     2924           
  Branches      229      229           
=======================================
  Hits         2207     2207           
  Misses        657      657           
  Partials       60       60

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbaef58...c8b7209. Read the comment docs.

efokschaner · 2017-07-19T11:04:00Z

Gotta fix up the apt-get installs to work in the container. I'll deal with that tomorrow as its late here in LA.

jdiaz5513

This is great. The build failure is the only thing blocking this, this otherwise looks good to merge as is.

Also, please add yourself to HUMANS.md if you like!

jdiaz5513 · 2017-07-19T15:55:33Z

package.json

    "codecov": "^2.2.0",
+    "glob": "^7.1.2",


Correct. Good catch.

jdiaz5513 · 2017-07-19T15:57:28Z

package.json

    "coverage": "gulp coverage",
+    "lerna": "lerna",


Doesn't hurt. Something like npx seems like the real answer.

jdiaz5513 · 2017-07-19T16:10:46Z

js-examples/addressbook.js

+}
+
+function main() {
+  // TODO Not sure if we have dynamic schema usage available yet


Dynamic schemas will likely never be a thing in this library. https://groups.google.com/d/msg/capnproto/lESKRE_pix8/ZB8ie0WuBgAJ

If you have a look at the C++ example https://github.com/capnproto/capnproto/blob/master/c%2B%2B/samples/addressbook.c%2B%2B#L264 it appears that here "dynamic" doesn't mean reading the schema at runtime. Rather I think it just pulls out the raw schema's from the compiled struct. eg. The member at: https://github.com/capnproto/capnproto/blob/cff543590ab25652ce56e9178dd499a8a69e08f7/c%2B%2B/src/capnp/schema.capnp.c%2B%2B#L689
so it can use it in a "dynamic" fashion. These schema's seem somewhat equivalent to our ObjectSize objects, but carry a bit more information.

Obviously in JavaScript we could just do things like:

function setFieldInCapnpStruct ( struct, fieldname, value ) { // ignore my lack of attention to the casing struct['set' + fieldname ](value); } function printAllCapnpFields ( struct ) { for(var prop in struct) { // I'm improvising here, my javascript is patchy, but you should get the idea if (/*has own property etc*/) { if(prop.startsWith('get') && struct[prop] instanceof Function) { console.log(prop.replace('get', '') + ' = ' + struct[prop].apply(struct)); } } } }

Which means having a dynamic API to the capnp data does not yield an increase in value anywhere near the same as in C++ with its meagre reflection tools out of the box.

However, I think it might still be preferable to support these api's if we can. As you'll still receive much better editor type information when doing things similar to the above example, and they'd be even simpler and more performant than what I've shown above. Also, note what I did above was super hacky and doesn't handle anywhere near enough edge cases which wouldn't even be a concern if you had the first class type information from capnp, so if someone tried the above in lieu of access to the compiled schema's it would be harder still.

Might be good to get @kentonv 's thoughts here too? What do you think about dynamic api support that doesn't involve parsing schema's at runtime?

A dynamic API is useful for writing generic algorithms that operate on arbitrary Cap'n Proto objects. For example, say you wanted to write a library for converting capnp types to/from JSON. This kind of thing usually requires schema information (maybe you can infer everything you need from native Javascript reflection, but often you can't). So your options are a dynamic API, or offline code generation.

Another use case that's particularly relevant to Javascript: It would be really cool to write a dynamic RPC interface explorer client. The client could connect to an RPC endpoint and dynamically request the interface's schema, then present a UI to the user to explore and manually invoke methods. In this case you don't even know the schemas at compile time, so a dynamic API is probably the only option.

This stuff probably isn't necessary for V1 but I think people will want it eventually.

💯 that clears up some ideas actually on how to implement toJSON/fromJSON. Upweights #45 since we'll need to have the schema compiler add even more metadata to the struct classes.

I'll write it in such a way that the schema object is easily serializable, and you can go from new CustomStruct() to new DynamicStruct(CustomStruct._capnpSchema)().

I'll write it in such a way that the schema object is easily serializable

I'd recommend embeding the actual capnp-encoded schema, which is what C++ and other code generators do. This way you can use the same code to dynamically process compiled-in types as well as schemas received dynamically over the wire, and you can also easily send your local schemas back out over the wire if desired.

jdiaz5513 · 2017-07-19T16:15:47Z

js-examples/test.sh

+# Used for release testing.
+
+set -exuo pipefail
+


You can avoid the need to cd to js-examples like so:

examples_dir=`dirname $0` capnpc -o $examples_dir/node_modules/.bin/capnpc-js addressbook.capnp

I'd have to add $examples_dir to all the local file references, no? addressbook.capnp, addressbook.js, etc?
What about we cd `dirname $0` ? It allows the script to get run from anywhere but the script still gets to assume its cwd.

Both would be fine.

jdiaz5513 · 2017-07-19T16:17:41Z

package.json

    "benchmark": "gulp benchmark",
    "build": "gulp build",
-    "ci": "gulp lint && gulp coverage && codecov --disable=gcov",
+    "ci": "gulp lint && gulp coverage && codecov --disable=gcov && cd js-examples && ./test.sh",


Yes. I'm also a bit reticent about having test.sh be UNIX only, since that means you need a *NIX box to work on this library.

Sigh maybe that's fine.

jdiaz5513 · 2017-07-19T17:57:13Z

packages/capnpc-js/src/index.ts

+/**
+ * The equivalent of tsconfig.json used when compiling the emitted .ts file to .js.
+ *
+ * TODO: This should be configurable somehow?


Let's drop this comment entirely. I'm fine with this not being configurable until someone can present a use-case for it.

Leaving a little about the motivation for the settings as they are.

jdiaz5513 · 2017-07-19T18:00:01Z

packages/capnpc-ts/src/generators.ts

+        ts.createPropertyAccess(THIS, '_getList'),
+        __,
+        [offsetLiteral, listClassIdentifier]);
+      init = ts.createCall(


I'll leave it up to you, but you might want to pull in this commit instead.

Not sure if you were expecting some fancy git tricks, but my git-foo is not the best, I'm just gonna add your changes to my own.

That's fine. I do have some git-fu for you though; add git@github.com:jdiaz5513/capnp-ts as a remote to your fork next time: https://help.github.com/articles/configuring-a-remote-for-a-fork/

Then after running a pull once you can easily cherry pick a commit:

git pull git cherry-pick 32df29647c3f9dc181b9b62b9d4eaef308fd0292

Slaps that commit as-is on top of your HEAD, and lets you resolve if there's a merge conflict.

jdiaz5513 · 2017-07-19T18:03:04Z

packages/capnpc-ts/src/index.ts


-        chunk.copy(reqBuffer, i);
+      const reqBuffer = new Buffer(chunks.reduce((l, chunk) => l + chunk.byteLength, 0));


I think I started the buffer byte length count at -1 because of strange behavior I was observing on OSX. 0 appears to be the correct starting point for the reducer, though, so this line is now correct.

I'll try to repro again later on OSX and come up with a patch that works across platforms if it's actually broken there. On my Arch box this works 💯.

Yeah this was breaking on osx for me (I was losing 1 byte at the end) which is why I changed it. I'm pretty sure that application code isn't meant to literally be passed an eof byte, pretty sure the underlying machinery in c runtime or somewhere is meant to turn it into a state change on the file descriptor and not feed it to you as data, I have no idea how you managed to receive an eof byte...

I think I misspoke on the c runtime thing. Sounds more like maybe you edited a binary codegenrequest in a text editor then piped it to stdin of the plugin directly and maybe it got an eof byte added by the editor?

Heyo! That's exactly what happened. 😁

Let's uh, leave it at 0 and pretend -1 never happened.

Clearly the capnproto spec just needs to allow trailing EOF bytes and we'd be good here.

efokschaner · 2017-07-20T05:09:24Z

packages/capnpc-ts/src/generators.ts

@@ -375,6 +383,8 @@ export function generateStructFieldMethods(

    default:

+      // TODO Maybe this should be an error?


Oh, I'd actually love your thoughts on this @jdiaz5513 ?

It should log a warning to console.error: "CAPNP-TS: Ignoring code generator request for unsupported struct field type: %s"

efokschaner · 2017-07-20T05:18:25Z

Welp, the apts capnproto is old too. Good thing i've seen this error before :P I'll rig it up to install the newest from source or something.

efokschaner · 2017-07-20T08:05:20Z

So the good news is that the CI works now! :if_only_gh_had_party_parrot:
The bad news is installing capnproto from source adds a couple of minutes at least. Have any thoughts on what we should do?

jdiaz5513 · 2017-07-20T23:44:43Z

It works? Ship it. Don't care if the CI takes 3 minutes longer. 30 mins would be nasty.

If you really want, look into making a debian package. It's easier than you think.

This introduces capnpc-js, and a new js-examples package that should be prominent within the repo, and have minimal dependencies to serve as a reference. It doubles as a full integration test of the system (although it's a little light on validation, as is the original in the C++ repo).

efokschaner added 4 commits July 15, 2017 19:20

WIP: capnpc-js and examples

ebbe798

Merge upstream master

e521e18

Get end-to-end working with capnpc-js

7936662

Merge upstream master

33f776a

efokschaner commented Jul 19, 2017

View reviewed changes

yarn.lock updated

b8b8e3c

efokschaner force-pushed the master branch from 710f4ed to b8b8e3c Compare July 19, 2017 10:45

Install capnproto in CI

88a4c4c

jdiaz5513 requested changes Jul 19, 2017

View reviewed changes

jdiaz5513 force-pushed the master branch from f695ac8 to 88a4c4c Compare July 19, 2017 18:20

efokschaner commented Jul 20, 2017

View reviewed changes

efokschaner added 2 commits July 19, 2017 22:12

Apply PR feedback

858f5d5

Yer' a ninja, Harry\!

615fa5c

efokschaner added 4 commits July 20, 2017 00:17

Get some capnproto in here please

82870f2

Make install without sudo

fcccb77

Fix install prefix

42e9238

Fix node 4.0

1c7c06d

node 4.0 pls

c8b7209

jdiaz5513 approved these changes Jul 20, 2017

View reviewed changes

jdiaz5513 merged commit 57f16f5 into jdiaz5513:master Jul 20, 2017


		chunk.copy(reqBuffer, i);
		const reqBuffer = new Buffer(chunks.reduce((l, chunk) => l + chunk.byteLength, 0));

		@@ -375,6 +383,8 @@ export function generateStructFieldMethods(

		default:

		// TODO Maybe this should be an error?

The official "addressbook" example working in JS #56

The official "addressbook" example working in JS #56

Conversation

efokschaner commented Jul 19, 2017

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

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

codecov bot commented Jul 19, 2017 • edited Loading

Codecov Report

efokschaner commented Jul 19, 2017

jdiaz5513 left a 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

efokschaner Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdiaz5513 Jul 20, 2017 • edited Loading

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

Choose a reason for hiding this comment

jdiaz5513 Jul 20, 2017 • edited Loading

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

efokschaner commented Jul 20, 2017

efokschaner commented Jul 20, 2017

jdiaz5513 commented Jul 20, 2017

codecov bot commented Jul 19, 2017 •

edited

Loading

efokschaner Jul 20, 2017 •

edited

Loading

jdiaz5513 Jul 20, 2017 •

edited

Loading

jdiaz5513 Jul 20, 2017 •

edited

Loading