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

Use nesting to represent nesting #44

Open
efokschaner opened this issue Jul 17, 2017 · 9 comments
Open

Use nesting to represent nesting #44

efokschaner opened this issue Jul 17, 2017 · 9 comments

Comments

@efokschaner
Copy link
Contributor

efokschaner commented Jul 17, 2017

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 efokschaner changed the title Use nesting to representing nesting Use nesting to represent nesting Jul 17, 2017
@jdiaz5513
Copy link
Owner

jdiaz5513 commented Jul 17, 2017

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.

@efokschaner
Copy link
Contributor Author

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

@efokschaner
Copy link
Contributor Author

efokschaner commented Jul 17, 2017

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

@jdiaz5513
Copy link
Owner

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!

@efokschaner
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@jdiaz5513
Copy link
Owner

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
Copy link
Owner

jdiaz5513 commented Nov 21, 2017

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
Copy link
Owner

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

The codegen will still be tough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants