Generate (sound) TypeScript and Flow declaration files

[lhchavez]

This change adds Flow declaration types so that we can check the types
of the generated code.

Notably:

• Adds a dependency to flowgen and calls it during compilation, which
  takes the .d.ts and creates a .flow.js based on it.

• Passes in the --force-number flag to pbjs, which causes it to not
  emit numbers as possibly being Long, since this confuses Flow. This
  only affects JSDoc declarations, not the generated code.

• Passes in the --force-message flag to pbjs. Otherwise, this emits
  unsound types: it tries to create code like this:

  interface IMessage {
    field?: T|null;
  }
  
  class Message implements IMessage {
    field: T;  // notice the lack of optionality / nullness.
    ...
  }

  This violates the Liskov Substitution Principle, since Message
  cannot be passed to any place that accepts an IMessage, so
  TypeScript (correctly) rejects this as bogus. With this flag, the
  number of places that will accept an IMessage instead of a Message
  are limited to constructors, where we do want to have partial messages
  being passed in.

  This only affects JSDoc declarations, not the generated code.

• Removes all @implements annotations from the JSDoc, which takes care
  of the very last IMessage/Message confusion. This, together with
  the above element take care of
  Generated TypeScript message class not compatible with generated message interface protobufjs/protobuf.js#837

[masad-frost]

• Passes in the --force-message flag to pbjs. Otherwise, this emits
  invalid types: it tries to create code like this:

  interface IMessage {
    field?: T|null;
  }
  
  class Message implements IMessage {
    field: T;  // notice the lack of optionality / nullness.
    ...
  }

  This violates the Liskov Substitution Principle, since Message
  cannot be passed to any place that accepts an IMessage, so
  TypeScript (correctly) rejects this as bogus. With this flag, the
  number of places that will accept an IMessage instead of a Message
  are limited to constructors, where we do want to have partial messages
  being passed in.
  This only affects JSDoc declarations, not the generated code.

This is actually valid typescript, and you can pass Message where it accepts IMessage since it fulfills the union. See https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgJIFkIGctwOYoDeAUMsjMBADYAmA-AFzJZhSh4A+IArlVQNzEAvsWIIqcHMkw58KYAFsADlQgKI4LGhm4CyEmQrUaTFmxB5kAXmQAiOLeGiY3EAjDAA9iGRg4Aa2wMbF0IAAp1WQImYKiIAEp9ETFvFmRI0OtkEAgAd2kQuTD4wT9ArFjQiMKCEqA

[masad-frost]

I'm not sure how I feel about this. There is no room for optional fields anymore inside our commands in ICommand, even though in protobufs all fields are optional. This is especially problematic for re-used messages such as File (maybe we should fix our protobufs).

For example, when you want to send a read command. You send a File message, you only fill the path field, like so (crosis):
channel.request({ read: { path: '.replit' } }), this will now fail because the read command under ICommand is now api.File|undefined instead of api.IFile|undefined, which means you have to do

channel.request({
  read: {
    path: '.replit',
    type: api.File.Type.REGULAR,
    content: new Uint8Array(),
    toJSON: () => ({ /* implement toJSON?*/}),
  },
});

or less foolish

const read = new api.File({ read: { path: '.replit' } });
channel.request({ read })

This might not be too bad but definitely cumbersome, and definitely requires a lot of code changes for us to get past typescript errors.

If ICommand used interfaces as well, this might be ok, but it doesn't, is there a way around that? Or do you think there's some way we can type this to support passing partial messages to channels https://github.com/replit/crosis/blob/v6/src/channel.ts#L123?

[masad-frost]

Also, added a boop label to the repo

[lhchavez]

• Passes in the --force-message flag to pbjs. Otherwise, this emits
  invalid types: it tries to create code like this:

  interface IMessage {
    field?: T|null;
  }
  
  class Message implements IMessage {
    field: T;  // notice the lack of optionality / nullness.
    ...
  }

  This violates the Liskov Substitution Principle, since Message
  cannot be passed to any place that accepts an IMessage, so
  TypeScript (correctly) rejects this as bogus. With this flag, the
  number of places that will accept an IMessage instead of a Message
  are limited to constructors, where we do want to have partial messages
  being passed in.
  This only affects JSDoc declarations, not the generated code.

This is actually valid typescript, and you can pass Message where it accepts IMessage since it fulfills the union. See https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgJIFkIGctwOYoDeAUMsjMBADYAmA-AFzJZhSh4A+IArlVQNzEAvsWIIqcHMkw58KYAFsADlQgKI4LGhm4CyEmQrUaTFmxB5kAXmQAiOLeGiY3EAjDAA9iGRg4Aa2wMbF0IAAp1WQImYKiIAEp9ETFvFmRI0OtkEAgAd2kQuTD4wT9ArFjQiMKCEqA

oh this is one of those cases where TypeScript is intentionally unsound D: see https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgJIFkIGctwOYoDeAUMsjMBADYAmA-AFzJZhSh4A+IArlVQNzEAvsWIIqcHMkw58KYAFsADlQgKI4LGhm4CyEmQrUaTFmxB5kAXmQAiOLeGiY3EAjDAA9iGRg4Aa2wMbF0IAAp1WQImYKiIAEp9UmRI0IA6I1prZFcaCAoQCBp+ZAB6Ut8AC2AtGuQANzgqYBpyTyhtELkAGmQAI24wZFBG5taYdukugjSnMW8WFOmUG0KAdym4sPjBP0CsWNCI5Z35kCxPVTSqTzxjuJ2gA for the rationale behind why this should be invalid TypeScript

Flow gets that right: https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVBLAdgFwKYBOUAhgMZ5gCSAsngM53EDmFA3qmGFBnjACYB+AFxg6OAtiYAfLAFcYMANyoAvulIxiDMLQbMKGALYAHGHkN5cdKrsYsw7Tt158RYiViZgAvGABExH7KaqhQslikOBhwWGA4xADW9DT0dngAFBZ6LCIp2XgAlA4cYFlpAHTO-D5g4Xx43Fh4fIpgwMBxABYY1j1gAG7EMBh8XHAENqn6ADRgAEayOGDYg8OjUOM6UyzlquoxYqXbFL5NSLb66QXK8Ul0eWmZx9eopAdwZuXwTE-510A and the fact that it is a flow compilation error is the reason why this was needed: when I tried to add this to the flow-typed server, it didn't like this unsoundness and errored in all the types.

changed the wording to say that the emitted code is "unsound" instead of "invalid" to avoid confusion.

[lhchavez]

I'm not sure how I feel about this. There is no room for optional fields anymore inside our commands in ICommand, even though in protobufs all fields are optional. This is especially problematic for re-used messages such as File (maybe we should fix our protobufs).

all fields in ICommand are still optional! the ones that are not optional are the Command ones. in other words, the interface that is used to be outbound types (ICommand) is still all-optional, but the interface that the decoded messages use (Command) has all default values filled in a priori.

For example, when you want to send a read command. You send a File message, you only fill the path field, like so (crosis):
channel.request({ read: { path: '.replit' } }), this will now fail because the read command under ICommand is now api.File|undefined instead of api.IFile|undefined, which means you have to do

channel.request({
  read: {
    path: '.replit',
    type: api.File.Type.REGULAR,
    content: new Uint8Array(),
    toJSON: () => ({ /* implement toJSON?*/}),
  },
});

or less foolish

const read = new api.File({ read: { path: '.replit' } });
channel.request({ read })

This might not be too bad but definitely cumbersome, and definitely requires a lot of code changes for us to get past typescript errors.

If ICommand used interfaces as well, this might be ok, but it doesn't, is there a way around that? Or do you think there's some way we can type this to support passing partial messages to channels https://github.com/replit/crosis/blob/v6/src/channel.ts#L123?

oh, this is a bit trickier, but i'll take a look. if a simple sed can't fix it, a Babel transformer will.

Added enough tooling to make the interface types also recursively accept interface types

[replbot]

unbooping: Your PR is too powerful! Try breaking it up into multiple changes.
If this is a pure refactor you can put [refactor] in the title.

[masad-frost]

Sweet, thank you! Hope this doesn't become a maintenance burden 😬

[masad-frost]

One worth checking is that these class properties that reference another message type are actually instances of the class not just the interface.

It doesn't seem to be the case, the types are wrong unfortunately https://repl.it/@masfrost/luisprotos

[lhchavez]

welp, seems like i've reached the limit of what can be monkeypatched from the outside. i'll probably have to send something upstream to avoid having to do some invasive surgery that's going to become a lot more work going forward.

In the interest of making forward progress, wdyt of adding a TODO and landing this mostly as-is? it'll take some time to get the change ready and approved and merged. worst case scenario, we fork that npm and patch it ourselves as a middle-term solution.

[lhchavez]

There's also a small change that can be done to comply with all the types: s/create/fromObject/: https://repl.it/@luisreplit/luisprotos#index.js, so by making .create() forward its parameters to .fromObject() we get the best of both worlds: the correct type checks for the parameters (thanks to .create()) and we create the correct type of the resulting object plus a little bit of validation (thanks to .fromObject()).

[masad-frost]

Sweet, bit spooky, but sweet. Thank you for powering through!

[masad-frost]

TBH I don't feel that great changing runtime semantics for the sake of types, but I don't wanna block this for too long.

From reading the code, fromObject has potentially non-trivial performance overhead. It's worth benchmarking this new code.

[lhchavez]

TBH I don't feel that great changing runtime semantics for the sake of types, but I don't wanna block this for too long.

From reading the code, fromObject has potentially non-trivial performance overhead. It's worth benchmarking this new code.

it's hard to say whether there is a statistically significant difference: I tried running benchmark in https://repl.it/@luisreplit/luisprotos#index.js and https://repl.it/@luisreplit/luisprotos-1#index.js (cannot load them into the same script because the generated code pollutes global state, so the last version require()d is the only one that is loaded -_- ), and they both oscillate between .6 and 1.4 Mops/sec.

in a rough apples-to-apples comparison, I pitted Command.create()/Command.fromObject() in the old code and new Command()/Command.create() in the new code and in most of the runs there's no statistically significant difference.