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

As implemented, module-scoped constants may not work properly

Open popham opened this issue 7 years ago • 4 comments

I noticed that your schema's constants get translated to naked values, e.g. capnpc-ts/test/integration/test.capnp.ts#L2110-L2145. Unfortunately, ES6 and Node's module resolution don't guarantee that imports are available at module scope. When you go to wrap an imported struct around some data for a module-scoped constant, you may discover that the imported struct is not yet available. Translating someConstant to getSomeConstant() generalizes to module scope without any problems, but I'm curious if you've got some other workaround.

popham avatar Dec 20 '18 02:12 popham

I might be missing something here, but isn't this only a problem when you have circular imports?

That's a thing to be avoided at all costs, and I certainly remember having to go out of my way to prevent it within this library.

A minimal example of how this might break would help.

On Wed, Dec 19, 2018, 6:53 PM Tim Popham <[email protected] wrote:

I noticed that your schema's constants get translated to naked values, e.g. capnpc-ts/test/integration/test.capnp.ts#L2110-L2145 https://github.com/jdiaz5513/capnp-ts/blob/524b6bde7d81037de586e2905d1d40955bbe22b0/packages/capnpc-ts/test/integration/test.capnp.ts#L2110-L2145. Unfortunately, ES6 and Node's module resolution don't guarantee that imports are available at module scope. When you go to wrap an imported struct around some data for a module-scoped constant, you may discover that the imported struct is not yet available. Translating someConstant to getSomeConstant() generalizes to module scope without any problems, but I'm curious if you've got some other workaround.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jdiaz5513/capnp-ts/issues/113, or mute the thread https://github.com/notifications/unsubscribe-auth/ACn_Z5fb8ldqWfEm0Opg4q3KmpVhk4Zyks5u6vuZgaJpZM4ZbZkH .

jdiaz5513 avatar Dec 20 '18 04:12 jdiaz5513

The problem will pop up in generated code. The Cap'n Proto schema language admits circular imports, so your generated code will encounter circular imports like the following:

// file1.capnp
const c1 :import "file2.capnp".F2 = ();
struct F1 {}
// file2.capnp
const c2 :import "file1.capnp".F1 = ();
struct F2 {}

If you're exposing value assignments as scoped constants, then a constant at a schema's file scope translates to an assignment at module scope, e.g.

//file1.capnp.ts
import { F2 } from "./file2.capnp";
export const C_1 = new F2...;
//file2.capnp.ts
import { F1 } from "./file1.capnp";
export const C_2 = new F1...;

When the module initializes, the F1 and F2 are not necessarily available for new.

I should monkey around with the code anyway; I'll submit a PR that amends packages/capnpc-ts/test/integration/import-bar.capnp and packages/capnpc-ts/test/integration/import-foo.capnp to demonstrate.

popham avatar Dec 20 '18 04:12 popham

Your struct-typed constants currently call capnp.rawPointer to yield a Pointer instance, so circular imports will work fine as currently implemented. If you intend to ever construct the actual struct types, however, then my proposed problem will arise. I was only able to look at struct-scoped constants because file-scoped constants did not get translated.

popham avatar Dec 21 '18 02:12 popham

I've added a hypothetical export of the module-scoped constant to #116. Running tests as written clobbered the hand coded additions, so I've altered gulp's test:capnpc-js task to stop the task from overwriting the hand-coded additions. Running gulp test:capnpc-js, note the error: import _foo_capnp_1.Foo is not a constructor. It chokes on the constructor before it even tries to work with a (null as any). Toss in a if (Foo===undefined) { throw new Error("PEEP"); } just before the const FOO =... line, and you'll get a "PEEP".

popham avatar Dec 21 '18 04:12 popham