-
-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add typescript definitions generation #135
Conversation
Hi @arthuro555, thank you for this awesome PR! This has definitely been a long time coming. I'll try to make some time this weekend to give this a proper review! 🙂 |
I haven't yet had a chance to give this a proper code review, but I was able to address some of the issues you raised:
I tried to fix these myself and and made a PR into your fork: arthuro555#1. With that change, the
I'm not familiar with "template types" in this context. Could you expand a bit on what you mean by that? Generally speaking, I think that
Totally fair! I appreciate you being careful and upfront about this. It'll take me some time to fully review the comment updates, but rest assured that I will do so. 🙂
Ultimately, I'd like to convert Shifty to being authored in TypeScript (and this PR certainly paves the way for that!), but that's a pretty significant change and I unfortunately I don't have the time to drive that forward right now. I don't think that a complete conversion to TypeScript would negatively impact runtime performance, as all of the type checking happens at compile time and never makes it into the actual runtime files. Assuming that there is no runtime performance impact, this would be much more preferable than maintaining separate sources of truth that need to be manually kept in sync. Although I don't have the bandwidth to actively develop Shifty these days, I can generally make time to help PRs like this along. If you'd like to submit more PRs and lead the conversion from JavaScript to TypeScript, you'd have my full support and I'll do what I can to help get PRs merged and shipped. For now, I think this PR is a major win all on its own and will benefit users of Shifty tremendously, so thank you for getting it started! |
Template types allow to specify a parameter to a type basically, for example: type MyType<MyTemplate> = {
myNumber: number,
myTemplatedType: MyTemplate,
}
MyType<string> == {
myNumber: number,
myTemplatedType: string,
}
MyType<{}> == {
myNumber: number,
myTemplatedType: {},
} They are especially cool since they can be inferred in some contexts: function noop<T>(s: T): T {
return s;
}
const a = noop("hello")
typeof a == string // Typescript infered without doing noop<string>("hello") that the return value is a string from the type of the parameter I actually did use templates for the interpolate function in this PR for an example of how they can be used here. The first advantage there is autocompletion, after writing the first argument, typescript infers the type and can provide high-quality autocompletion for the second argument, as it knows what keys to autocomplete and for each key what type is associated with it. The second advantage is that the return value is the exact type of the input value as well. For example, if I typed state with console.log(interpolate({a: 1}, {b: 2}, "linear").c); All TS knows is that those are objects keyed with strings and numbers, but with templates we can tell it that they should have the same shape, and TS will warn about a missing in the second parameter, b missing in the first one, and accessing c in the return value which would not exist there.
Thanks for offering your support! I'll try to look into contributing to the switch to TypeScript if I find the time :) |
Interesting! Template types sound like a powerful mechanism for defining data constraints. Thank you for the thorough explanation! Do you think that this is this something that would be available to us if Shifty was fully converted to TypeScript? As a stopgap interim solution, do you think the
For sure! Thanks for contributing to the project. Just let me know what you need from me! 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! Thank you for putting in the time and work for this.
There's just a few small things to address and they are commented inline. Also I opened arthuro555#2 for your fork. When that's merged, we'll be able to properly test the documentation locally (it was broken from before this PR)!
@arthuro555 was there anything else you think this needs before it can be promoted to full PR and then merged? This seems to be the only outstanding issue that I can see: #135 (comment) If you think this is ready to go, let's promote this out of Draft state and then I can merge it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ready to merge. Incredible work! Thank you for adding these TypeScript types. This is a huge improvement to Shifty and I know other devs will appreciate the work. I definitely do! :)
Once this is merged to develop
, I'll make a minor
release for this and get it out to everyone!
@arthuro555 was there anything else you think this PR needs before merging? I typically let PR authors do the actual merge in case there's anything they wanted to wrap up last-minute. If you'd prefer, I can merge myself, so just let me know! |
I don't think I have anything else to do in that PR, I do not have repository write permission so I cannot merge it myself. |
Oh I'm sorry! I though you had merge permissions since the PR was approved. Sorry for the delay. I'll merge this now! |
I just tried releasing this as I'm 99% certain that this is unrelated to this PR, but I will need some time to fix this. I'm guessing that a dependency is out of date and is suddenly manifesting itself as a broken build. 😕 |
This is now available in |
This adds automatic generation of TypeScript type definition files for shifty from the JSDoc comments.
There are a few issues:
Those could likely be fixed by either switching the whole codebase to typescript (preferably not as the transpiling process might have unwanted side effects on performance) or creating the types manually and manually update them on any changes to the public API (makes maintenance just a bit more painful). Do you think one of those would be a better option?
Closes #68