Add babel macro

[zenflow]

This is based off my branch for PR #23 since it uses format: "js:cjs" under-the-hood. I believe when that PR is merged, those commits will stop appearing in this PR.. If I'm wrong I can rebase and fix it for review.

I'm testing it out with a little demo project and it works beautifully! I am very happy! I will share it via codesandbox after a bit more work. Maybe this can be included as a link in the README.

There are still some issues, a couple of which I need help with:

1. instead of just makeCheckers(...): ICheckerSuite, also offer edit buildTypeSuite(...): ITypeSuite, to give the option of more control to users, and so they can possibly build & compose/merge multiple type suites and call t.createCheckers directly with the result.
2. should never inline the same checker suite expression more than once in same file
3. split lib/index.ts into lib/{Compiler,bin,macro,index}.ts - Now that the CLI is not the only consumer of the Compiler, we should reorganize things a bit before merging. I suggest keeping a single entry-point for the build (i.e. lib/index.ts), as it is now, to keep things simple. That entry-point would just export the exports of Compiler.ts, bin.ts, and macro.ts. @dsagal Does this sound ok? If so, I will make the change just before merging. Not needed. Made a separate entry-point for the macro.
4. discuss: brings in ts-interface-checker as devDependency.
   This is a notable change I thought would be worth bringing up.
   I think it makes sense and even we might want to do some integration testing on the ts-interface-builder side.
   The motivation here is that the TypeScript checking for the related test fixtures (which my IDE does automatically)
   breaks when the ts-interface-checker package is not installed, due to its import in macro.d.ts.
   It's not strictly necessary, but it's nice to see that everything is compiling & working as expected.
   @dsagal What are your thoughts?
5. add ability to makeCheckers('some-npm-package')? Not strictly necessary since you can already import {SomeThing} from 'some-npm-package'; and then use makeTypes().SomeThing (no first argument to makeTypes means build the current ts module).
6. issue: test/test_index.ts tests the source (i.e. lib/**), while test/test_macro.ts tests the build (i.e. dist/**). Can that be fixed so both test the source?
7. documentation updates

[zenflow]

FYI the only relevant commit (ie. not from PR #23) here right now is 754f3ef.

[dsagal]

This is based off my branch for PR #23 since it uses format: "js:cjs" under-the-hood. I believe when that PR is merged, those commits will stop appearing in this PR.. If I'm wrong I can rebase and fix it for review.

I squashed-merged #23 , so you'll need to rebase, but it looks like maybe you already did.

I'm testing it out with a little demo project and it works beautifully! I am very happy! I will share it via codesandbox after a bit more work. Maybe this can be included as a link in the README.

Yes, definitely good to include instructions and an example in the README.

There are still some issues, a couple of which I need help with:

1. [ ]  instead of just `makeCheckers(...): ICheckerSuite`, also offer `buildCheckers(...): ITypeSuite`, to give the option of more control to users, and so they can possibly build & compose/merge multiple type suites and call `t.createCheckers` directly with the result.

I think I understand what you mean. The naming of createCheckers, makeCheckers, buildCheckers is hard to tell apart. If for buildCheckers you mean building just the "type suite", i.e. {Type1: t.iface(...), Type2: etc} then maybe better to call it buildTypeSuite or buildTypeDescriptors. I agree the flexibility it adds could be useful. In a way, it's adding runtime type introspection that's missing in TypeScript!

Actually, buildTypeSuite may be sufficient. Then usage would be

import {createCheckers} from 'ts-interface-checker';
import {buildTypeSuite} from 'ts-interface-builder/macro';

interface Foo { ... }

const checkers = createCheckers(buildTypeSuite(undefined, {}));
checkers.Foo.check(input);

Might this be clearer? Keeping makeCheckers as essentially a shortcut to the above is fine too.

2. [ ]  should never inline the same checker suite expression more than once in same file

Sounds sensible.

3. [ ]  split `lib/index.ts` into `lib/{Compiler,bin,macro,index}.ts` - Now that the CLI is not the only consumer of the `Compiler`, we should reorganize things a bit before merging. I suggest keeping a single entry-point for the build (i.e. `lib/index.ts`), as it is now, to keep things simple. That entry-point would just export the exports of `Compiler.ts`, `bin.ts`, and `macro.ts`. @dsagal Does this sound ok? If so, I will make the change just before merging.

Splitting lib/index.ts sounds good.

It seems fairly easy to avoid a dependency on babel-plugin-macros for those users who don't need it (e.g. I don't use babel at all). If we keep macro-specific code to lib/macro.ts (can be exposed as top-level ./macros, to be importable as ts-interface-builder/macros), then it could just fail in case importing babel-plugin-macros fails, and tell the user to install it (as would the README instructions).

Would that be OK?

4. [ ]  discuss: brings in `ts-interface-checker` as `devDependency`.
   This is a notable change I thought would be worth bringing up.
   I think it makes sense and even we might want to do some integration testing on the `ts-interface-builder` side.
   The motivation here is that the TypeScript checking for the related test fixtures (which my IDE does automatically)
   breaks when the `ts-interface-checker` package is not installed, due to its import in `macro.d.ts`.
   It's not strictly necessary, but it's nice to see that everything is compiling & working as expected.
   @dsagal What are your thoughts?

Bringing in ts-interface-checker as devDependency for testing and better editing of text fixtures sounds fine.

The rest of the checklist sounds like TODOs to yourself.

Thanks again! I agree this is quite cool. I only wish I could do it within typescript :)

[zenflow]

@dsagal Thanks for the prompt initial review! Great to have feedback early on. Also thanks for reading and addressing the issues I listed!

so you'll need to rebase, but it looks like maybe you already did.

Yup I rebased it as soon as I saw the other PR was merged 👍

Yes, definitely good to include instructions and an example in the README

Definitely. The codesandbox I have is less of an example of "How to use" and more of live demo showing it in action in a realistic scenario, so I will include both a short/minimal "How to use" example, and a link to the codesandbox demo. Here it is:
https://codesandbox.io/embed/github/zenflow/ts-interface-builder-macro-demo/tree/main/?module=%2Fpages%2Fapi%2Fmath.ts&hidenavigation=1&fontsize=14

Since there will be a link to it in the README (if that's ok), can I get you to take a quick peek at it and give any feedback? P.S. I'm ok to move the repo (zenflow/ts-interface-builder-macro-demo) to the gristlabs org if you want.

If for buildCheckers you mean building just the "type suite",

As, yes, I meant buildTypeSuite(...): ITypeSuite, not buildCheckers(...): ITypeSuite. Terribly sorry for any confusion it may have caused before you figured out what I meant.

Yeah, so if we have buildTypeSuite, then makeCheckers isn't strictly necessary, but I think it will be a nice little shortcut, since
it does what I think users will want 90% of the time.

Of course, in the README, I will start with documenting buildTypeSuite, since it is the foundation, and then document that makeCheckers(...) is equivalent to createCheckers(buildTypeSuite(...)).

Splitting lib/index.ts sounds good

Excellent. We can do this just before merging, to minimize the chance of merge conflicts (in case another PR with changes to lib/index.ts is merged before this PR).

It seems fairly easy to avoid a dependency on babel-plugin-macros [...]

Ah yeah, totally. I didn't think it would be such a big dependency as it turns out, and just included it because other babel macros do.

Let me work on this and I'll give you an update in a bit.

Bringing in ts-interface-checker as devDependency for testing and better editing of text fixtures sounds fine.

Great! I can mark the "discuss" item as done now.

[zenflow]

It seems fairly easy to avoid a dependency on babel-plugin-macros [...]

@dsagal Fixed with ed8633d. I moved the macro to lib/macro.ts and made a separate entrypoint of it, to produce dist/macro.js, so the dist/index.js can be required without requireing babel-plugin-preval.

Regarding my 2nd TODO ("split lib/index.ts"): It turns out it's not at all complicated to have multiple entrypoints in the dist, and seems like the correct way to do it.

So I guess we should have src/{Compiler,main,macro,index}.ts & dist/{Compiler,main,macro,index}.js?

Or can we make a possibly breaking (but not likely breaking) change to organize things better? We could drop the index files and make dist/Compiler.js the package's main module. (Main module would no longer export the main() function.)

[dsagal]

Definitely. The codesandbox I have is less of an example of "How to use" and more of live demo showing it in action in a realistic scenario, so I will include both a short/minimal "How to use" example, and a link to the codesandbox demo. Here it is:
https://codesandbox.io/embed/github/zenflow/ts-interface-builder-macro-demo/tree/main/?module=%2Fpages%2Fapi%2Fmath.ts&hidenavigation=1&fontsize=14

Since there will be a link to it in the README (if that's ok), can I get you to take a quick peek at it and give any feedback? P.S. I'm ok to move the repo (zenflow/ts-interface-builder-macro-demo) to the gristlabs org if you want.

The codesandbox example seems pretty cool. As far as feedback, I guess it depends on your main goal. Just to showcase the functionality, it's a bit big -- you have to know what to look for to notice in the code the the validation provided by the building + checking of interfaces. We could have a much smaller example, but I agree that this one is more realistic in that validation's main use case is when there is communication.

My main "review" comment for that example code is to add explanatory comments -- particularly to the code that's relevant to using the new macro.

I'd say you should keep it as your own repo. We'll link from the instructions. In fact, if you prefer, you can also create your own repo for ts-interface-builder-macro so that you are the owner of that code (and I'd be happy to link to it from ts-interface-builder README), but the way you've set it up now -- with no extra dependencies -- works fine too.

[zenflow]

In fact, if you prefer, you can also create your own repo for ts-interface-builder-macro so that you are the owner of that code

@dsagal Yeah, I wouldn't mind making it a separate repo & package under my ownership. It would be a nice gem for my GitHub profile and it would allow me to maintain it directly without always the need to bother you. Its also kinda the more modular way to do it. The main practical benefit that I see there is that each (ts-interface-builder and ts-interface.macro) would have their own README documentation, and a higher level of organization in general.

My only reservation is that as part of the ts-interface-builder package, the macro's tests are guaranteed to be passing whenever you make changes to ts-interface-builder. This is only a concern because the macro's implementation relies on on the Compiler to output a very specific (but very simple) format for format: 'js:cjs'. The output should be two statements: ... t = require("ts-interface-checker"); and module.exports = { ... };.

Should we add another option for format like "js:expression" that just outputs the type suite expression without that commonjs wrapper?

Or should we just agree that the format: 'js:cjs' output won't change from how it's described above?

[zenflow]

@dsagal Ok I rebased all my work onto my branch for #26 (which is based on the current master). Two commits:

• Add "test-only" package script 4cd6baf
• Add babel macro, closes Feature: Provide Babel Macro to avoid extra build step #22 93a30a0

[zenflow]

@dsagal Really sorry I ended up rewriting the whole commit history for this PR... I really get annoyed when people do this to me.. (it kinda invalidates the changes I already reviewed, and I have to review everything all over again) so I don't know why I did it here. 😛

I'll restore the branch with it's original history now, and push a new branch with all updates & a condensed commit history: add-babel-macro-2

Let me know if you would like me to apply those updates to this (original) branch so you can easily see what's changed. I'm happy to do that, even if we decide this will be a separate package, just to use GitHub PR to get your code review.

Sorry again.

[zenflow]

The codesandbox example seems pretty cool. As far as feedback, I guess it depends on your main goal. Just to showcase the functionality, it's a bit big -- you have to know what to look for to notice in the code the the validation provided by the building + checking of interfaces. We could have a much smaller example, but I agree that this one is more realistic in that validation's main use case is when there is communication.

My main "review" comment for that example code is to add explanatory comments -- particularly to the code that's relevant to using the new macro.

@dsagal I added some explanatory comments, but yeah I think you're right that it's a bit overkill.

I'll make a different, more minimal, live working example when I'm writing the documentation. Maybe a similar interface, but just checking the user's JSON input in the browser (no API requests)? And using a simpler example TypeScript interface?

[zenflow]

I only wish I could do it within typescript :)

@dsagal That made me sad to hear, so I did a bunch more research.

As it turns out, this could be implemented this as a TypeScript transformer instead of a babel macro, which you might be able to use, if you can substitute the tsc CLI command with ttsc, or if you use the TypeScript compiler programatically. This doesn't work for all situations obviously (just the aforementioned situations), but could cover some situations, just like the babel macro.

BTW I hit a gold-mine of other projects that do things similar to ts-interface-checker/builder: https://www.reddit.com/r/typescript/comments/i8yk6i/validating_objects_type_at_runtime/g1cleg3/

[dsagal]

BTW, my delay in responding wasn't to nudge you towards making it a separate repo :) Just regular being swamped :)

Should we add another option for format like "js:expression" that just outputs the type suite expression without that commonjs wrapper?

Or should we just agree that the format: 'js:cjs' output won't change from how it's described above?

I have no problem with either. I kind of like js:expression idea (except that the mention of undeclared t would be a bit awkward, so maybe it should output a function(t) { return {... t.iface(...) ...}; }?) But at the same time, if js:cjs works for you, I think we could keep it from breaking with a little test case containing a comment what it's protecting against.

No worries about rebasing. I'll re-review from scratch, since it's been too long to be fresh in my mind, and a lot of code moved around between files anyway.

As it turns out, this could be implemented this as a TypeScript transformer instead of a babel macro, which you might be able to use, if you can substitute the tsc CLI command with ttsc, or if you use the TypeScript compiler programatically. This doesn't work for all situations obviously (just the aforementioned situations), but could cover some situations, just like the babel macro.

Hmm, interesting... Maybe some day someone will do that. Seems promising.

BTW I hit a gold-mine of other projects that do things similar to ts-interface-checker/builder: https://www.reddit.com/r/typescript/comments/i8yk6i/validating_objects_type_at_runtime/g1cleg3/

That's a lot of projects :) Well, I am glad you still like this one! :)

OK, on to reviewing.

[dsagal]

Looks really good, thank you!

[zenflow]

I'll cross these 2 items off the TODO list in my opening comment:

add ability to makeCheckers('some-npm-package')? Not strictly necessary since you can already import {SomeThing} from 'some-npm-package'; and then use makeTypes().SomeThing (no first argument to makeTypes means build the current ts module).

Like the original text explains, this isn't really necessary. I don't think it's worth the bit of complexity either.
The inline docs for getCheckers & getTypeSuite now say that the module path should be a relative one.

issue: test/test_index.ts tests the source (i.e. lib/), while test/test_macro.ts tests the build (i.e. dist/). Can that be fixed so both test the source?

The macro module is loaded by babel-plugin-macros and is expected to be JS (not TS), so there's not really any way to fix this on our end. We just can't have babel-plugin-macros load the TS source; we need to compile it to a JS build first.

[zenflow]

@dsagal Well it's a tough decision for such an arbitrary one, but I think that the babel macro has more of an advantage being part of this package rather than being separate. As long as you don't mind that the macro doubles the size and complexity of this package. I'm confident we can do a good job maintaining it together.

Assuming that you're still cool with that (merging this PR to make the babel macro part of this package) then I think all that's left to do is close out the above threads and update the README? Anything else you'd like to add?

[dsagal]

I am cool with that, and since the code changes seem finished, I went ahead and merged. Changes to README can go into a separate commit.
Thank you @zenflow ! This is quite an exciting feature!

[zenflow]

@dsagal Awesome! I'll follow up with the README updates and then I guess we can publish a new version 🚀

It makes sense to update the README for ts-interface-checker too right? That README seems to have the overall usage instructions for ts-interface-builder+ts-interface-checkers.

[dsagal]

Agreed. I suggest adding a separate documentation file with a detailed description of the macro usage, with examples, babel config, etc. And in the ts-interface-checker main README, include an overview with brief example, just enough to appreciate it and use it, and link to the detailed file. The bit in ts-interface-builder could literally be a one-liner with a link to the overview ts-interface-checker README and to the detailed doc.

[zenflow]

@dsagal I was looking at ts-interface-checker's README and how to just "update" it in regards to the addition of the macro (not just insert a new section). I came up with gristlabs/ts-interface-checker#31. I feel like going into detail (like usage examples) about both methods (cli & macro) is a lot of extra info that actually belongs to ts-interface-builder. I think even the part in my PR that describes the 2 methods (cli & macro) is not really necessary, since all that is really necessary is to to say "use ts-interface-builder to get type suites". What do you think?

Regarding README updates in this repo (ts-interface-builder), it seems that the README for the most part (the whole "Usage" section) could be rewritten/reorganized (with all the current information) such that we describe the purpose & usage & behavior of the package in general (things that apply to both the CLI & the macro) & then have section (or file) for the CLI usage & a section (or file) for the macro usage.
In other words.. It seems like good organization to separate "cli" documentation from "core" documentation, now that there is a difference. What do you think?

[dsagal]

I responded to the ts-interface-checker proposal in that PR. As for "cli" vs "core" documentation, it sounds sensible, but looking at ts-interface-builder's README today, there is pretty much only "cli", so I am not really sure what you mean by "core". E.g. an example of a type suite could be core, but it's not really visible to the user when using the macro.

In general, I think a good README is one that can be read incrementally -- start with what's it about, and how to get started quickly (some readers in a rush will stop there), give some examples (for readers looking for more clarity), then have more sections with more details (for those who need it). Now that there are two different ways to get started quickly, I think it's still good to present them briefly near the beginning, then give more explanation and the details of each method.

[zenflow]

but looking at ts-interface-builder's README today, there is pretty much only "cli", so I am not really sure what you mean by "core". E.g. an example of a type suite could be core, but it's not really visible to the user when using the macro.

Yep, an example of a type suite would be "core". Even though it's not visible in any file when using the macro, it's still every bit as relevant for the macro user as it is for the cli user. As a macro user you generally want to know how it works, and what the output looks like. The type suite example isn't strictly necessary in either case (cli or macro) (with the cli, the user could just import & use the generated module without ever looking at the contents) but is helpful for understanding how the system works.

So I think that info (example type suite for a given TS module) can be covered in the intro section (and also included in examples in the CLI Usage & Macro Usage sections).

There is not much else under "Usage" that isn't talking specifically about the CLI.. just a few statements in the first paragraph like "This module works together with ts-interface-checker module". I want to move those to the intro section.

Since you seem to agree with the principle of separating "core" documentation & cli documentation & macro documentation (and since we're definitely on the same page regarding your second paragraph), I will try a PR to see what you think :)

[zenflow]

So I think that info (example type suite for a given TS module) can be covered in the intro section (and also included in examples in the CLI Usage & Macro Usage sections).

You know what? Screw that.. we can keep "type suite" an abstract idea in the intro section, and make it concrete in both CLI Usage & Macro Usage. The intro section just needs to introduce the idea of a type suite.. no example is necessary for a high-level understanding.