-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
[@testing-library/vue] New type definition #37655
Conversation
Travis build failed with an error about the dependency whitelist, but I already have a PR merged in types-publisher for that: microsoft/types-publisher#643. Is there something else that needs to happen on the DefinitelyTyped side to get that change?
|
👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics. Let’s review the numbers, shall we? These typings are for a package that doesn’t yet exist on master, so I don’t have anything to compare against yet! In the future, I’ll be able to compare PRs to testing-library__vue with its source on master. Comparison details 📊
If you have any questions or comments about me, you can ping |
@sandersn would you be able to advise on the types publisher issue please? |
@cimbul Thank you for submitting this PR! Because this is a new definition, a DefinitelyTyped maintainer will be reviewing this PR in the next few days once the Travis CI build passes. In the meantime, if the build fails or a merge conflict occurs, I'll let you know. Have a nice day! |
@cimbul The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
||
// Reexports event functions from dom-testing-library | ||
// Note that it does NOT make the core fireEvent() function asynchronous | ||
lib.fireEvent(elem, new Event('change')); // $ExpectType boolean |
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.
edit: okay, just read your sentence of the PR. My bad!
AFAIK, fireEvent()
is returning a Promise (source), and here it appears to be returning a boolean
disclaimer: I might be missing something basic, as I have no experience with type definitions 😇
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.
Yeah, I'll get this updated to 2.0 after we get 1.2 published. (Or I could just do that now if we want to avoid another PR—I don't feel that strongly about it.)
@johnnyreilly caching on Travis isn't aware of updates from github references in package.json. So I have to manually invalidate the cache until I figure out how to to fix that. |
Awesome; this just proves the truth of Phil Karlton's maxim 😁
|
@sandersn @johnnyreilly It looks like basically everyone's PR builds are failing since this got merged in... 😬 |
@cimbul thanks for the warning. Turns out my cache invalidation yesterday didn't take?? Anyway, everything should be fine now. |
This is for version 1.2. Version 2.0 was recently released two days ago, but the types will require minimal changes. I think it makes sense to get the types for this version published before making the update.
npm test
.)npm run lint package-name
(ortsc
if notslint.json
is present).If adding a new definition:
.d.ts
files generated via--declaration
– See Add typings testing-library/vue-testing-library#74 and fix(typescript): move typings to DefinitelyTyped testing-library/react-testing-library#437dts-gen --dt
, not by basing it on an existing project. Didn't work with scoped package, so I adapted it from othertypes/testing-library__*
definitions.tslint.json
should be present, andtsconfig.json
should havenoImplicitAny
,noImplicitThis
,strictNullChecks
, andstrictFunctionTypes
set totrue
.