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

[WIP] feature(rpc): generate classes for interfaces (non-generics) #91

Closed

Conversation

couchand
Copy link

@couchand couchand commented Mar 2, 2018

Starting to hash out what it would look like to generate classes for RPC interfaces in schema definitions. Still a fair amount of work to be done before this is mergeable (finishing up interface references, tests, squashing commits), but I thought it would be good to get your feedback on it so far. It does build, typechecks, and produce reasonable-looking code for basic interfaces.

The approach I've taken is to hew pretty closely to the C++ implementation. It seems like that's more or less the strategy taken elsewhere, so it's a well-worn path.

No attempt is made at runtime support (other than basic enough types to pass the typechecker). Probably a fair amount will need to change with the generated code based on things learned while implementing the runtime, but this seems like a good step.

I'm interested to hear what you think of it so far.

@couchand couchand mentioned this pull request Mar 2, 2018
21 tasks
@jdiaz5513
Copy link
Owner

This is a wonderful start! You picked up on conventions used elsewhere in the code quite handily.

I'm going to start leaving some feedback. Please push back if it becomes overly pedantic.

@@ -68,6 +68,27 @@ export function createNestedNodeProperty(node: s.Node): ts.PropertyDeclaration {

}

export function createObjectLiteral(properties: { [key: string]: ts.Expression }): ts.Expression {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, actually just a note to myself: capnpc-ts is in the middle of a pretty heavy upgrade, so this will require careful merging. Should not attempt to rebase my working branch from here.

return 'capnp.Interface';
const i = getFullClassName(lookupNode(ctx, type.getInterface().getTypeId()));

// TODO: is this snippet relevant?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but will be overwritten soon. The compiler is getting some nice upgrades.

import { Foo } from './interface.capnp';

tap.test('interface generation', (t) => {
t.ok(new Foo.Client() instanceof capnp.Capability_Client);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great initial test case! Nice and simple. 👍

@jdiaz5513
Copy link
Owner

Still a fair amount of work to be done before this is mergeable

I will somehow resist the urge to play with this until tests are in. RPC is the reason why I started this library!

The approach I've taken is to hew pretty closely to the C++ implementation. It seems like that's more or less the strategy taken elsewhere, so it's a well-worn path.

Indeed following the reference implementation was the way to go for me, for the most part. The go implementation is also quite good and I reference it from time to time for ideas. The templating in C++ is… a good kind of crazy. :)

Don't dive deep into capnpc-ts since it's getting a pretty big upgrade right now, which I'm on the verge of finishing. That will mark v0.3.0, the first version that will be able to compile this monster without 🌋 all over itself.

Tests started failing on master to some dependency nonsense, so don't sweat the CI status for now. If this annoys you enough to inspire a separate PR against master, I would not protest!

@couchand
Copy link
Author

couchand commented Apr 3, 2018

Sorry to go silent on this. I went off to try to understand the C++ implementation and got in deep. Then I started to look into the Rust implementation, that seems to have some interesting ideas that would be reasonable to borrow into the JS world. In any case, it seems like there will be a fair amount of big design to this project. I remain convinced that compiling reasonable-looking types is the right first step, and I think prototyping a strawman runtime would be a good next step.

What's your status with the 0.3 changes? I wouldn't want to go too far if there's going to be a lot of churn from that.

@virtulis
Copy link

virtulis commented Sep 1, 2018

Hi guys.

So, it looks like I'm not the first one with idea to use capnp to interact with wasm :)

I'm working on a project so let's say I'm very eager to help get this done. Problem is I'm not very familiar with the spec or the reference implementation so I'm not entirely sure how the end result should look (and whether you'd be satisfied with what I can come up with).

So I'm wondering I could chat with one of both of you somewhere to figure out if and how I could be useful and how much actual work is left - if you have time for that :)

@jdiaz5513
Copy link
Owner

I'll be taking a closer look at it this weekend. I have to really dive in and wrap my head around the reference implementation for RPC; I may then pick this up and reintegrate it with master.

I'll likely be able to come up with a desirable API design for this, and leave unimplemented methods that others can jump in to backfill if you're still looking to help.

fasterthanlime added a commit to fasterthanlime/capnp-ts that referenced this pull request Apr 4, 2019
@popham
Copy link
Contributor

popham commented Jul 9, 2019

I'm just beginning work on an RPC runtime for a Flow annotated version of capnp over at https://github.com/capnp-js. Rather than writing the interface generation code (serialization generation and runtime are already done), I've decided to handwrite some "generated" interfaces, develop the runtime and tweak the handwritten stuff in parallel, and then generate the interface stuff last. Has anybody around here already written some runtime code that I can poach? Has anybody contemplated how to implement capability reference counting in a garbage collected language like JavaScript?

@couchand
Copy link
Author

couchand commented Jul 9, 2019

I'm just beginning work on...

@popham perhaps you could focus efforts on adding flow annotations and RPC to this library? The serialization code here is well tested and currently in use in production systems.

Has anybody around here already written some runtime code that I can poach?

I suspect that it would be hard to share very much, since it depends so much on the architectural specifics.

Has anybody contemplated how to implement capability reference counting in a garbage collected language like JavaScript?

Pretend like it doesn't have GC, just like with any perf-sensitive JS code. Make sure you keep handles to things you don't want to disappear. You need the handle anyway if there's any hope of getting the thing back. Maybe I don't understand what you're asking?

@couchand
Copy link
Author

couchand commented Jul 9, 2019

@jdiaz5513 have you had a chance to take a look at this recently?

@popham
Copy link
Contributor

popham commented Jul 9, 2019

@couchand, sorry dude, but I'm intent on my own stuff. I've got generics support (I've got major changes to push from my local repo, but this is the gist of it), and my generator anticipates Maybe support that I expect the reference implementation to add someday (see capnproto/capnproto#633 (comment)).

On the runtime specifics, I was hoping for data structures (e.g. questions table) and control logic, but putzing around forks of this repo I didn't see anything. I figured that watchers of this PR were the most likely people to have something laying around. If not, then my work should translate into this project with minimal grief.

On the GC, I'm probably thinking too far ahead. Cap'n Proto capabilities use reference counting, so I anticipate needing a strategy for removing a capability from the RPC system if its export count is zero. I'm more recalling thinking about it in the distant past, i.e. https://gist.github.com/popham/de943ad3d89116c9c553#file-calc-js-L43, but I'm not sure if my caution is warranted since it was so long ago. I was using ReactJS a lot at the time, so I may have been projecting their component setup and tear down architecture onto Cap'n Proto.

zenhack pushed a commit to zenhack/capnp-ts that referenced this pull request Jun 19, 2021
@jdiaz5513 jdiaz5513 force-pushed the master branch 3 times, most recently from 3f0375a to 64778e0 Compare August 19, 2021 04:43
zenhack pushed a commit to zenhack/capnp-ts that referenced this pull request Aug 19, 2021
@jdiaz5513
Copy link
Owner

Closing this in favor of #166 which is starting to look like it's going to work. Thank you for getting it started!

@jdiaz5513 jdiaz5513 closed this Jan 6, 2022
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.

4 participants