-
Notifications
You must be signed in to change notification settings - Fork 404
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
feat: native typescript support #246
Conversation
Codecov Report
@@ Coverage Diff @@
## master #246 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 21 23 +2
Lines 281 529 +248
Branches 68 124 +56
==========================================
+ Hits 281 529 +248
Continue to review full report at Codecov.
|
We used to have this (sort of, not implemented in TypeScript, but type definitions in this package), and in #123 it was decided to move them to DefinitelyTyped because they did not work out of the box in VSCode for auto-completion and things like that. But I very much welcome this change. Thanks for all the effort. I'd rather have the library implemented in TS so you doing this is very much welcome! Now the question is, does this satisfies the issues raised in #123? |
If the TypeScript defs are a part of the package then it will work out of the box (and arguably better). FWIW, DOM Testing Library just moved the types into the repo: testing-library/dom-testing-library#530 |
One more thing: we may have some PRs merging soon, and with this kinds of changes, the conflicts are usually hell. For instance, I'm considering merging #244 which is an entirely new feature, but it will block this PR having to rebase and rewrite that new matcher as well. Given all the effort you already put into this, and how much I want this to happen, would you rather have this merged first and then we can require that PR to rewrite to TS (which does not have to be the author, I am willing to step in if needed). Or do you think it's ok and we can take care of the rebasing and rewriting in this PR? |
Ok, that's what I suspected. This has full green light then. Not sure what to merge first then, if this or #244. |
@gnapse I would say merge #244 first and then I'll rebase on top of it. @kentcdodds Nice! The difference is that this is a complete rewrite to Typescript (opposed to just pulling the types into the package) so there is no overhead of editing types -- they are auto generated. Also this comes with a subtle change to how matchers are written internally -- to successfully generate correct types arguments have to be taken of the function's |
Thanks! Let me know if you need help with the rebase or rewrite, if you do not have time soon. I'd be more than happy to help giving this the final push. |
That may happen one day, but as noted in that PR I'm not certain of it. We'll see. |
…into jest-dom-in-typescript
@kentcdodds any idea why |
Could be a bug. It's not a feature I've used much yet so I'm not sure. |
Yeah its definitely a bug, if I manually copy local |
@gnapse is it possible to do alpha pre-release so I can confirm this works from few of my projects. I linked the package locally and it worked, but still better be safe than sorry. |
To be honest, not at all sure how to go about doing an alpha release. I've always relied on automated releases. @kentcdodds can you chime in on this? |
Make a branch called alpha and merge it into that branch. Then make sure Travis builds that branch and it should work. |
@Meemaw I created the Though doing this won't actually make an alpha release. We'll have to go with releasing directly. @kentcdodds not sure if this is breaking change or not. Seems it is not, but, is it a feature? (i.e. a minor version release) or a fix (patch release)? I'm leaning towards feature, though technically there's no new feature here, but also no fix. |
I think you should actually name it I expect it will make a pre-release like this: https://github.com/testing-library/react-testing-library/releases/tag/v10.0.0-beta.2 |
|
Fell free to merge. I would consider squashing it into a fix/patch commit. |
Yes, we always do that. |
🎉 This PR is included in version 5.7.0-beta.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
I’m just catching up on this - apologies for the delay. But won’t this change prevent types from being globally available without some kind of import statement? If you recall discussion from #123, TypeScript will only apply global typings automatically if they are in the I believe this change will be a regression there since it drops the |
I will make up some time to checkout this version of the lib locally to make sure. Did you do that @jgoz (TBH that's the only way I know, I do not know TypeScript enough to be sure of this without actually trying it out, I only know the language but I'm rusty on all its setup kind of things like this). BTW @kentcdodds said above the opposite
which is what I'd expect, because I normally consume libraries written natively in TypeScript and they work out of the box without the need for the |
Hmmm, I'm very unhappy to report that indeed, this seems to be a regression, which I don't understand. Isn't it supposed that if the library is in TS it should all work? Anyway, here's what I did:
Isn't there a way to make this work satisfying #123 and having the code in TS @jgoz? |
@gnapse I don't believe it's possible, which is why we went down the DefinitelyTyped route in the first place. Writing the library natively in TypeScript has no impact on how the types are consumed by users compared to JavaScript + type definitions. The big difference is internal typings vs. DefinitelyTyped typings. Our case is a special one because the API is used in a global context (the import statement/setup is detached from actual usage). For a normal library where something is imported and used in the same file, supplying your own types or converting to TypeScript makes sense (@kentcdodds is correct about that). But that is not how people use this library and TypeScript does not automatically include global type definitions from random packages in So this isn't all bad news, we have a couple of options to work with here. Instead of reverting this entire PR, we could just revert the It might also be possible to simply import our internal typings from the DT typings so we didn't have to keep them in sync, but that might result in circular |
@jgoz I was thinking and had an idea that might work. Wha about a postinstall script that would move typings to types folder? We get best of both worlds. It is possible to install packages with arugument to ignore those, but I think that is very rare. |
@Meemaw That is an interesting idea. I think as long as the script was fairly defensive (create directories as necessary, don't overwrite files) and it always operated relative to the user's Making it relative to the user's |
Wow, that sound cool.
And also, nobody can complain if they then do not have the expected type awareness support in their IDE or somewhere else. |
I'll try that out as soon as I find some time. |
@gnapse I'm a bit noob-sauce on TS but trying 5.7.0-beta.1 didn't work on my project. I added but my test still complains about only thing that seems to work for me is adding this to my tsconfig.json:
|
@benmonro this is a known issue, exactly what we're discussing above (see #246 (comment) above). This is precisely why we released this only as a beta release for the time being. And the immediate discussion prior to your comment was about an approach that @Meemaw is looking into to solve this. |
gotcha ok wasn't sure but thanks for clarifying! |
@gnapse any update on this? |
This is waiting on checking up if this idea proposed by @Meemaw could work:
|
Update: |
@Meemaw @jgoz how about having jsdoc-based type checking? I recently had a very good experience introducing it in a project that other reasons was very difficult to take directly to TypeScript. I'm so excited about this, that I'd be willing to have that at least as a second-best outcome from this effort. I'd work on this myself since @Meemaw already laid the groundwork here. |
@gnapse would that be an alternative to rewriting the library in TypeScript? Certainly doable if you prefer to keep things in JS, but it won’t help us export global typings. I think DefinitelyTyped is still the best way to provide typings if we are striving for zero-configuration. |
What:
Add native Typescript support to
jest-dom
.Why:
How:
All files were converted to Typescript and global declaration added which extends
jest
matchers:Checklist:
This PR is not ready to be merged yet, there is still some polishing to be done if we want to merge it. However, I would like to get some feedback if going into that direction is even desired.