Skip to content

Add typings #74

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

Closed
wants to merge 1 commit into from
Closed

Add typings #74

wants to merge 1 commit into from

Conversation

cimbul
Copy link

@cimbul cimbul commented Aug 9, 2019

Followed recommendations in dtslint tool and TypeScript documentation.

Took tsconfig.json and tslint.json from @testing-library/dom, although it looks like they just copied the examples in the dtslint docs.

Closes #60.

Followed recommendations in [dtslint tool](https://github.com/microsoft/dtslint) and [TypeScript documentation](http://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html).

Took tsconfig.json and tslint.json from @testing-library/dom, although it looks like they just copied the examples in the dtslint docs.

Closes testing-library#60.
@codecov
Copy link

codecov bot commented Aug 9, 2019

Codecov Report

Merging #74 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #74   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          61     61           
  Branches       10     10           
=====================================
  Hits           61     61

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 b98df9b...ce7b824. Read the comment docs.

@testing-library testing-library deleted a comment from codecov bot Aug 10, 2019
@testing-library testing-library deleted a comment from codecov bot Aug 10, 2019
@afontcu
Copy link
Member

afontcu commented Aug 10, 2019

Thank you very much for this!

I think we should wait before merging this since there are several substantial changes incoming (such as #75 and the DOM Testing Library version bumping).

Moreover, I just noticed this comment regarding typings and its maintainability. I have little to no experience with TypeScript, so I wouldn't be able to keep typings up to date. What are your thoughts about the proposed solution by Kent?

@cimbul
Copy link
Author

cimbul commented Aug 10, 2019

No problem. Yeah, we could put this on DefinitelyTyped if that's the direction that dom-testing-library is going. It doesn't change the maintainability situation, but it does mean you won't be responsible for fielding issues for it.

For what it's worth, since these definitions only express the interface, changes like #75 don't impact them much. The only relevant change I can see is that the main fireEvent() function now returns a promise. These types don't use many advanced TypeScript features, but the ones for dom-testing-library do (BoundFunction being one), so I can see how that might make things harder.

@afontcu
Copy link
Member

afontcu commented Aug 12, 2019

For reference, typings from DTL and RTL are moving to DefinitelyTyped:

testing-library/dom-testing-library#337
testing-library/react-testing-library#437

@nickserv
Copy link
Member

nickserv commented Aug 12, 2019

The TypeScript team recommends that packages not written in TypeScript have their types in DefinitelyTyped (@types on npm) instead of in the same package: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

You should be able to reuse most of this if you'd like to make a pull request to DefinitelyTyped, though they have some additional tooling and code style requirements.

@cimbul
Copy link
Author

cimbul commented Aug 13, 2019

I'll submit the DefinitelyTyped PR once the dom-testing-library typings get moved there. (Otherwise it's a whole process to add @testing-library/dom to a whitelist, only to then remove it.) I'll close this and add a comment when I get that PR submitted.

@afontcu
Copy link
Member

afontcu commented Aug 13, 2019

Thanks @cimbul ❤️

@cimbul
Copy link
Author

cimbul commented Aug 15, 2019

DefinitelyTyped PR here: DefinitelyTyped/DefinitelyTyped#37655

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

Successfully merging this pull request may close these issues.

Npm installation error (VueJS/Typescript)
3 participants