capnp-ts icon indicating copy to clipboard operation
capnp-ts copied to clipboard

Use nesting to represent nesting

Open efokschaner opened this issue 8 years ago • 9 comments

I noticed that in our codegen we transform nested things into top-level members with an underscore between parent and nested member. eg. CodeGeneratorRequest_RequestedFile Whereas (for example) in the official capnproto C++ compiler they generate an actually nested RequestedFile struct inside the CodeGeneratorRequest struct.

See: https://github.com/jdiaz5513/capnp-ts/blob/master/packages/capnp-ts/src/std/schema.capnp.ts#L989 Vs: https://github.com/capnproto/capnproto/blob/1dcabbf81d772069119773025d44c29df19ccd30/c%2B%2B/src/capnp/schema.capnp.h#L626

I think it would be more intuitive to match the C++ style and nest emitted things when they're nested in the schema, if there's no barrier to doing so in TS that I'm unaware of.

efokschaner avatar Jul 17 '17 06:07 efokschaner

Yeah, I fought with this pretty hard. In TS you can't nest types, unfortunately. You can nest objects, but the result isn't as nice as you'd hope:

class B {

  constructor() { ... }

}

class A {

  static readonly B = B;

}

// This is fine.
const x = new B();

// Also fine.
const y = new A.B();

// Compiler error. :(
let z: A.B;

After thinking about it a lot, I realized it's best not to even nest the structs at all since I figured people were going to hit that wall pretty quick and be confused why that last line won't compile.

Maybe this is a thing to bring up to the TypeScript team? Possibly an existing issue already?

At the very least this caveat should be documented loudly.

jdiaz5513 avatar Jul 17 '17 14:07 jdiaz5513

Still scratching my head over why that last line would be an error. Pity, as it seems quite natural.

efokschaner avatar Jul 17 '17 16:07 efokschaner

Does this have the same issues you described? https://stackoverflow.com/a/32494175

efokschaner avatar Jul 17 '17 17:07 efokschaner

Yep that's basically the same problem. It's extra gnarly when you need to export the class with the nested type.

I think I made an attempt to solve it using namespaces and gave up.

If you're feeling adventurous, try editing schema.capnp.ts manually and see if you can get it to compile with an exported namespace and nested types!

jdiaz5513 avatar Jul 17 '17 19:07 jdiaz5513

I never got my head around TS namespaces. I've mainly only had bad experiences with them.

In this case anyway, can a namespace double as a class? Because the outer name still needs to be the outer type.

Regardless, sounds like you've done a lot of exploration but I might have a play around with this anyway.

efokschaner avatar Jul 18 '17 03:07 efokschaner

This does look promising:

export class A {
    constructor(public a: string) {}
}

export module A {
    export class B {
        constructor(public b: string) {}
    }
    export module B {
        export class C {
            constructor(public c: string) {}
        }
    }
}

let a = new A('a');
const b = new A.B('b');
const c = new A.B.C('c');

// No compiler errors. :)
let bb: A.B = b;
let cc: A.B.C = c;

console.log(bb);
console.log(cc);

But maybe you already tried this and know how it might fail?

Note I went with module vs. namespace, the subtleties of TypeScript's namespaces and modules elude me but I've run into horrible issues with multiply-defined symbol errors resulting from namespace-introduced global symbols interacting poorly with npm-linked modules in precisely the kind of lerna setup we're using here. So I'm hoping that using module might allow us to avoid that issue but I'm not certain and we should check for that specifically before adopting. Can provide more details if you're interested.

I'm not gonna try and adapt the schema / code generator to do this yet as I want to get current work on capnpc-js squared away but I'm happy to come back to this.

efokschaner avatar Jul 18 '17 06:07 efokschaner

I think I remember trying that, and I believe you run into use-before-declare issues with self-referencing types.

Notwithstanding the fact that writing the codegen for that is going to be a doozy.

It's worth trying again, though.

jdiaz5513 avatar Jul 18 '17 15:07 jdiaz5513

Namespaces may work without a major headache. They even get around some of the use before define issues I run into elsewhere.

export class Foo {
    static foo = 'foo';
    bar: Foo.Bar;
    constructor() {
        this.bar = new Foo.Bar();
    }
}

export namespace Foo {
    export class Bar {
        static bar = 'bar';
    }
    export class Fotz extends Foo.Bar { }
}

let x: Foo.Bar = new Foo.Bar();

I'm weary of the past horrors with namespaces but am willing to take the dive; the compiled schemas are pretty lacking without proper nested types.

jdiaz5513 avatar Nov 21 '17 06:11 jdiaz5513

This needs further exploration now that TypeScript has advanced a bit.

The codegen will still be tough.

jdiaz5513 avatar Aug 29 '18 03:08 jdiaz5513