First cut at typescript-lite generator implementation.

[eboto]

This is a first attempt at lightweight TS courier binding generation. See the README for what exists and what doesn't.

Been working on this since Monday so I wanted to release some of the code for early review & input.

I've tested these manually against my own .pdsc files as well as the reference-suite of .courier files and it's at the point that all suites compile and visually look good. Next step is to write an npm build that exercises the reference types in runtime. When that satisfies then I'd like to return and clean the code up a bit.

Definitely looking for any input on the approach, pitfalls, omitted features, etc. Also I definitely don't feel confident about when ClassTemplateSpecs should be used, vs the internal DataSchemas. I could be doing some boneheaded things here and there so I'd like to understand more about when one is called for vs the other.

UPDATE

Just finished cleaning everything up, adding more test cases, etc. This code is ready for a real review and hopefully a merge!

[eboto]

Made a few changes since first posting:

• Embraced module-per-file approach. So org.example.Fortune will have its own file org.example.Fortune.ts, which you import with import { Fortune } from "./my-bindings/org.example.Fortune
• Patched a few bugs on Arrays of enclosed union types.
• Integrated ts-lite generated bindings into my project to convince myself that they work well. It was a smashing success!

Going to go ahead and start writing a test-suite now. When that's done I'm going to return to the code and clean it up a bit.

[saeta]

@eboto Sorry, I'm not familiar with the idioms of typescript. Can you provide a rationale for not including UNKNOWN$ in this list?

[eboto]

Sure! We use UNKNOWN$ at runtime in existing client libraries to surface the case when an unknown enum value, probably from a later version of the software, appears down the wire. However, the enums as written here (string-literal types) do not have a run-time component so it would be impossible to coerce that unknown string into UNKNOWN$ as part of the type. So I chose not to include it in the type.

That pretty much leaves it on the client to compare to all the KNOWN enums, and let the unknown one fall off the end. e.g. if you have enum Fruits { APPLE, ORANGE } then the client would have to do

const fruit: Fruit = /* get the fruit */
if (fruit == Fruit.APPLE) { 
  // something
} else if (fruit == Fruit.ORANGE) {
  // something
} else {
  // handle unknown
}

Now thinking about it though, I could provide a utility function like Fruit.isUnknown(enumValue: Fruit) that returns boolean if we got an unknown value. But I'm not sure how much that would help the experience.

Did that make sense? What do you think?

[saeta]

Looks pretty reasonable. Given that they're represented as strings, it should be reasonable to use them in a switch statement as well. Having the enum variants generated I think is the right call. I'd expect they'll gzip really well, such that the overhead of including them won't be much higher than having a bunch of string constants in the source code. (And this way, you get autocomplete, and the potential for tooling to better understand what's going on in the future.)

[eboto]

Good point! Don't know why I reached for this gross if-elses first.

I'm going to give the switch usage a workup in the testsuite then update the docs.

According to this typescript issue, we will get compiler warnings future versions for strings that are not in the string literal type...so that will make it even nicer (as long as it doesn't stop you from falling through to default.

EDIT: Also I think I agree with you about the constants down the wire. I'm going to keep them. =)

[saeta]

Hey @eboto I've taken a super quick look over. I think you're definitely on the right track. I'll try and take the time to give a more thorough review over the next few days.

I'm excited to see this come in, and land successfully. Thanks for your great work already!

[eboto]

I introduced some test cases in the form of an NPM build that processes the reference suite. Though it only tests that the type system accepts correct input now, I would like to additionally put in some tests to that show we don't accept the wrong input. That's a little messier though here because the data's shape is verified at compile time.

I do have some outstanding questions relating to testcases that wouldn't pass:

• The test cases are not exhaustive: they pretty much only test the example .json files that were already contained in the reference suite. Based on your knowledge of the space, do these test cases need to be further expanded before merge?
• What is a TypedDefinition and do you feel that this implementation needs to support them?
• Regarding TSProperties.java:37, what usage pattern are we moving away from? Maybe a question for @jpbetz
• I assume that the parts of namespaces are generally separated by dots. Is that a fair assumption?
• What is a Flat Type and do you feel that this implementation needs to support them?

Pending a more thorough review, my only remaining tasks AFAIK on this issue are:

• Address TypedDefinition and Flat Types if necessary
• Come to a conclusion on how to handle UNKNOWN$ as per your comment in the PR
• Introduce some negative test cases
• Clean up the cruftalicious code

[eboto]

Hey @saeta I was writing test-cases trying to use the existing json objects in reference suite, when I came across FortuneCookie.json. I assume it's used as a positive example, but the extra fields "map" and "simple" seem to disqualify it from the courier definition. Any idea why it is included as a positive example? Are supersets of a courier record supposed to be valid examples of the courier record as well?

Doesn't have much bearing on typescript-lite bindings anyways but I'm just curious.

[eboto]

OK I'm done with the reference test suite. Let me know if the suite is up to snuff!

[eboto]

Alright I'm done here, @saeta ! I'm sure we'll have a handful more patches later on, and I'd love to add a more appropriate formatter, but hey I think this is a decent starting point.

May be worth noting that performing fulltest now requires that you have npm installed on your machine.

[saeta]

:-)

[eboto]

Thanks for the review, Brennan! Lots of good food for thought. So summing it all up it seems like these are the remaining issues! Does this sound accurate to you?

• Do README workup and test-case of switch/case for enums
• Remove backticks from WithAnonymousUnionArray
• Double-check typed maps behavior and assert it in the README
• Patch string escaping to use $ instead of _
• Put Projections workup into README
• Commit to including the string constants in the enum, and remove references to removing them

[eboto]

@saeta I've hit, I think, all the points I intended to. Do you reckon this thing is ready for a merge now?

Also curious what you thought of the projection solution?

[eboto]

@saeta ping! Are there any additional steps to take before getting this initial merge in?

[saeta]

Hey @eboto I'm sorry for the delay. I think this is in good shape for the initial merge. Thank you very much!