woutervh- / typescript-is

MIT License
959 stars 35 forks source link

ts-auto-guard collab? #13

Open rhys-vdw opened 5 years ago

rhys-vdw commented 5 years ago

Hey @woutervh-. Looks like we're both working on similar tools. I would have emailed you privately but I can't find your contact details.

Maybe it would make sense to work together instead of building the two tools in parallel?

I've got a debug mode that will explain why the check failed. You've got some of my planned features (assertions etc). Generally speaking I think I prefer your API and your project seems more mature than ts-auto-guard. Without having looked I assume you've got a nicer code base too. ;)

Let me know what you think.

woutervh- commented 5 years ago

Hi @rhys-vdw

Thanks for contacting me, I think it can be useful for us to collab. Sounds like we are working towards the same goal indeed.

I had a quick look at ts-auto-guard and the approach seems a bit different, but it includes some of my planned features (generating a file with named functions, better debug logging) as well as some features I hadn't thought about, but want to add now (CLI) :)

Specifically the idea of emitting a new file with the type guards instead of "inlining" the type guards is something I've wanted to do from the beginning (https://github.com/woutervh-/typescript-is/issues/7#issuecomment-459270021)

How do you think we should start the collab?

rhys-vdw commented 5 years ago

@woutervh- I'm not sure exactly how your inlining works, but I actually thought this was one of the smarter decisions you made (at least in terms of user ergonomics).

For example, when using ts-auto-guard I need to generate the guards before I can use them, slowing down development with compile errors. Whereas is<T>() is always available because it's exposed by your library's types.

Basically at the moment I'm integrating ts-auto-guard into the UsabilityHub codebase, spending a couple of hours a week on it. I could start putting that energy into your project instead.

You'll see that ts-auto-guard kind of naively concatenates a big string to generate files, rather than operating directly on the AST, so unfortunately I don't think I'll be able to copy code wholesale. But I could try to add in some features that you're missing.

The important features, for my intended usage, are the "short circuit" feature, and detailed debugging. I use guards to validate data from our Rails backend before it hits TypeScript. These checks can get quite big, as you can see. I don't want this code in production, just in CI and development to let developers know when they're failing. Also, because they're so big they really need to report which keys are failing.

Let's talk about these features and whether they make sense for this project.

woutervh- commented 5 years ago

hi @rhys-vdw

True, inlining makes it easier for the user to work with the package. The downside is, though, if you use the type guards in multiple lines of code (or across multiple files), the type guards for the whole set of referenced types is generated again. So it can result in some unnecessarily big files.

I'm currently doing quite a big refactor (branch recursive-types https://github.com/woutervh-/typescript-is/tree/recursive-types) in order to make a lot of things easier to do in the future: https://github.com/woutervh-/typescript-is/issues/2 (debugging messages) https://github.com/woutervh-/typescript-is/issues/7 https://github.com/woutervh-/typescript-is/issues/9 https://github.com/woutervh-/typescript-is/issues/11 https://github.com/woutervh-/typescript-is/issues/12 (primary focus of the branch)

In particular the first issue (#2) is still a WIP and with this refactor should give much friendlier debug messages. It will give much better detail about what failed the validation.

I think it should be relatively straight-forward to implement this "short circuit" feature in the current master branch as well as in the branch that I'm doing the refactoring in.

I think both of the features you mentioned make sense for this project.

Feel free to submit PRs or if you're not sure how to do it I can also do it or give some pointers.

rhys-vdw commented 5 years ago

True, inlining makes it easier for the user to work with the package.

Right, well there are other ways to do it. For example, you could generate a giant switch statement inside of is<T> that is reused from different contexts.

e.g.:

function is<T>(obj: any, uniqueTypeSpecifier: string): obj is T {
  switch (uniqueTypeSpecifier) {
    case "./path/to/module:exportName":
      return (...)
    case "./path/to/other/module:exportName":
      return (...)
  }
}

The nice part is just that is<T>() will be a valid function call regardless of whether the backing code has been generated. Calls could be updated like this:

is<Foo>(maybeFoo)

// becomes 

is<Foo>(maybeFoo, "types/foo:Foo")

And then the is function can be generated on the fly. The nice thing about your approach (as opposed to mine) is that you don't have to keep the generated code under version control, and you don't need to remember to update it when you redefine your types, and if debug logging or short circuiting features were added, you wouldn't need to manually recompile the guards.

In fact, you don't even need to rely on uglifyJS for short circuit in this package, you just generate a simple return true implementation for each guard.

woutervh- commented 5 years ago

hi @rhys-vdw

Yes, I've thought about creating such an is<T> function that is used globally. My idea was to dump a map of FQN to type guard like so:

function is(obj: any, fqn: string) {
  const mapping = {
    "string": (obj) => typeof obj === "string",
    "./path/to/module:exportName": (obj) => ...,
    ...
  };
  return mapping[fqn](obj);
}

However when I tried this approach, I got stuck on: how do I use the transformer to modify the existing node_modules/typescript-is/index.js file? Also, often people don't include node_modules in their artifacts and when they deploy their application the node_modules comes from doing an npm i, so then it would be lost. Then the only way (I can think of) you could ship the generated type checks is with some generated file at compile time, as you do with ts-auto-guard, or by inlining it like in this project.

Anyway I've just finished my large refactoring effort so the debug messages are a bit friendlier. When I have some more time I will also add this short-circuit feature :)

Was there anything else you would like to see in typescript-is?

woutervh- commented 5 years ago

Hi @rhys-vdw

I've added support for a shortCircuit option: https://github.com/woutervh-/typescript-is#options It's available in version 0.10.2 Let me know if it's working for you or not.

rhys-vdw commented 5 years ago

Hey @woutervh-, just letting you know I'm currently overseas, but I'll be back and able to collaborate in April. :)

rhys-vdw commented 5 years ago

@woutervh- I'm back, and will try this out.

0xorial commented 5 years ago

Just as a small user feedback - I was recently trying to add checks to my angular project and approach from ts-auto-guard was A LOT easier to integrate (I am working on an angular app).

rhys-vdw commented 5 years ago

@0xorial oh really? That's interesting to hear. I've just put this on ice entirely because I'm using it to surface JS errors in our Rails test suite and there are so many that it's turned into a multi-month process to get CI up and running on the branch haha.

hmil commented 4 years ago

@0xorial Did you say that because angular's build system is so obscure and integrating a build-time transformer involves 3rd party build tools and hacking into private members of the angular toolchain?