Skip to content
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

Merged
merged 9 commits into from May 7, 2020

Conversation

Meemaw
Copy link

@Meemaw Meemaw commented May 7, 2020

What:

Add native Typescript support to jest-dom.

Why:

  1. To prevent issues with unsynced/wrong type declarations, e.g. typescript declaration for ToHaveValue doesn't allow null #240
  2. To make the developer experience better (no need to update multiple repors -- types will be created for free)
  3. For type safe code

How:

All files were converted to Typescript and global declaration added which extends jest matchers:

import * as extensions from './matchers'

expect.extend(extensions)

type Extensions = typeof extensions

declare global {
  namespace jest {
    interface Matchers<R, T> extends Extensions {}
  }
}

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions 💣 -- not needed anymore
  • Ready to be merged

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.

@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #246 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/index.ts 100.00% <ø> (ø)
src/matchers.ts 100.00% <ø> (ø)
src/extend-expect.ts 100.00% <100.00%> (ø)
src/to-be-checked.ts 100.00% <100.00%> (ø)
src/to-be-disabled.ts 100.00% <100.00%> (ø)
src/to-be-empty.ts 100.00% <100.00%> (ø)
src/to-be-in-the-document.ts 100.00% <100.00%> (ø)
src/to-be-in-the-dom.ts 100.00% <100.00%> (ø)
src/to-be-invalid.ts 100.00% <100.00%> (ø)
src/to-be-required.ts 100.00% <100.00%> (ø)
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 943a0c9...8a6dc04. Read the comment docs.

@gnapse
Copy link
Member

gnapse commented May 7, 2020

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?

@gnapse gnapse requested review from jgoz, gnapse and kentcdodds May 7, 2020 18:14
@kentcdodds
Copy link
Member

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

@gnapse
Copy link
Member

gnapse commented May 7, 2020

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?

@gnapse
Copy link
Member

gnapse commented May 7, 2020

If the TypeScript defs are a part of the package then it will work out of the box (and arguably better).

Ok, that's what I suspected. This has full green light then. Not sure what to merge first then, if this or #244.

@Meemaw
Copy link
Author

Meemaw commented May 7, 2020

@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 arguments pseudo array, see https://github.com/testing-library/jest-dom/pull/246/files#diff-f2f42330cea8557c48c9c5ef1084da8aR82

@gnapse
Copy link
Member

gnapse commented May 7, 2020

I would say merge #244 first and then I'll rebase on top of it.

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.

@kentcdodds
Copy link
Member

That may happen one day, but as noted in that PR I'm not certain of it. We'll see.

.travis.yml Outdated Show resolved Hide resolved
@Meemaw
Copy link
Author

Meemaw commented May 7, 2020

@kentcdodds any idea why tsc --noEmit would be failing from kcd-scripts/husky pre-commit hook? Running tsc --noEmit locally in terminal passes for me, which makes me think the pre-commit one doesn't take some things into account (tsconfig.json?)

@kentcdodds
Copy link
Member

Could be a bug. It's not a feature I've used much yet so I'm not sure.

@Meemaw
Copy link
Author

Meemaw commented May 7, 2020

Yeah its definitely a bug, if I manually copy local tsconfig.json into node_modules/kcs-scripts/dist/config it works. Its not respecting the user's �tsconfig.json.

@Meemaw
Copy link
Author

Meemaw commented May 7, 2020

@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.

@gnapse
Copy link
Member

gnapse commented May 7, 2020

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?

@kentcdodds
Copy link
Member

Make a branch called alpha and merge it into that branch. Then make sure Travis builds that branch and it should work.

@gnapse gnapse changed the base branch from master to alpha May 7, 2020 20:03
@gnapse
Copy link
Member

gnapse commented May 7, 2020

@Meemaw I created the alpha branch and set it as the base for this PR, so let me know when you're ready and we can merge it and see how it goes.

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.

@Meemaw
Copy link
Author

Meemaw commented May 7, 2020

I think you should actually name it beta to match branches travis builds on: https://github.com/testing-library/jest-dom/blob/master/.travis.yml#L18

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

@gnapse gnapse changed the base branch from alpha to beta May 7, 2020 20:11
@gnapse
Copy link
Member

gnapse commented May 7, 2020

beta created and this branch changed base to it

@Meemaw
Copy link
Author

Meemaw commented May 7, 2020

Fell free to merge. I would consider squashing it into a fix/patch commit.

@gnapse
Copy link
Member

gnapse commented May 7, 2020

I would consider squashing it into a fix/patch commit.

Yes, we always do that.

@gnapse
Copy link
Member

gnapse commented May 8, 2020

🎉 This PR is included in version 5.7.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jgoz
Copy link
Collaborator

jgoz commented May 8, 2020

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 @types folder. Otherwise, you (as a user) need to have an import statement in your code or add to your tsconfig include/files for types to be available.

I believe this change will be a regression there since it drops the @types dependency. It doesn’t matter if the types are written manually or generated by TypeScript, they will not be globally available without some kind of user configuration, which is what we were trying to solve in #123.

@gnapse
Copy link
Member

gnapse commented May 8, 2020

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

If the TypeScript defs are a part of the package then it will work out of the box (and arguably better).

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 @types dependency (or maybe they have it internally and I've never checked 🤔) Anyway, I'll come back with my findings.

@gnapse
Copy link
Member

gnapse commented May 8, 2020

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:

  1. Took a project I'm already working on and upgraded the version of jest-dom to v5.7.0-beta.1.
  2. Deleted node_modules/ (as a safety measure, to be sure I was starting clean) and ran npm install
  3. Made sure that with this the dependency @types/testing-library__jest-dom is no longer present.
  4. Restarted vscode and opened a test file that is not written in TypeScript. Then tried to type a new assertion. There is no auto-complete available for jest-dom matchers anymore 😕
  5. Reverted it all to how it was (v5.7.0), now auto-complete in .test.js files works again.

Isn't there a way to make this work satisfying #123 and having the code in TS @jgoz?

@jgoz
Copy link
Collaborator

jgoz commented May 8, 2020

@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 node_modules. It will only include them from node_modules/@types, maybe for performance reasons, maybe to avoid conflicts/confusion/etc.

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 package.json change and re-add the @types dependency. And then we could use the .d.ts output in this repository as the source of truth and manually synchronize it with the global typings in DefinitelyTyped. Not ideal, but it would work.

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 npm dependencies and might need special intervention from the DT maintenance team again since it is likely against the rules.

@Meemaw
Copy link
Author

Meemaw commented May 8, 2020

@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.

@jgoz
Copy link
Collaborator

jgoz commented May 8, 2020

@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 package.json file, it could work well.

Making it relative to the user's package.json is important because this library might be a transitive dependency of another package and so might get installed deeper in the node_modules hierarchy. But @types packages will only get picked up automatically if they are in the top-level node_modules folder, nearest to the user's package.json file.

@gnapse
Copy link
Member

gnapse commented May 8, 2020

Wow, that sound cool.

It is possible to install packages with arugument to ignore those, but I think that is very rare.

And also, nobody can complain if they then do not have the expected type awareness support in their IDE or somewhere else.

@Meemaw
Copy link
Author

Meemaw commented May 11, 2020

I'll try that out as soon as I find some time.

@benmonro
Copy link
Member

benmonro commented May 11, 2020

@gnapse I'm a bit noob-sauce on TS but trying 5.7.0-beta.1 didn't work on my project.

I added setupFilesAfterEnv: ['./jestSetup.ts'], to jest.config.js and import "@testing-library/jest-dom"; to jestSetup.ts

but my test still complains about .toBeInTheDocument() unless i import jest dom in the test file. am i missing something?

only thing that seems to work for me is adding this to my tsconfig.json:

    "../../node_modules/@testing-library/jest-dom/extend-expect.d.ts"

@gnapse
Copy link
Member

gnapse commented May 11, 2020

@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.

@benmonro
Copy link
Member

gotcha ok wasn't sure but thanks for clarifying!

@benmonro
Copy link
Member

@gnapse any update on this?

@gnapse
Copy link
Member

gnapse commented May 19, 2020

This is waiting on checking up if this idea proposed by @Meemaw could work:

a postinstall script that would move typings to types folder

@Meemaw
Copy link
Author

Meemaw commented May 19, 2020

Update:
I've tried that approach and it works perfectly (on first install). It seems postinstall script is not triggered when you add packages or something else and then the typing folder that was initially created will get cleaned up...

@gnapse
Copy link
Member

gnapse commented May 30, 2020

@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.

@jgoz
Copy link
Collaborator

jgoz commented May 30, 2020

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants