-
-
Notifications
You must be signed in to change notification settings - Fork 44
Conversation
Thanks for working on this! Since I’m not very familiar with typescript, I’d like to get some feedback from @TAGraves before we merge. Really appreciate the contribution! |
src/index.d.ts
Outdated
type Bound<T> = T extends (arg: any, ...rest: infer U) => infer V ? (...args: U) => V : never | ||
type BoundQueries<T> = { [P in keyof T]: Bound<T[P]> } | ||
|
||
export type FiberRoot = Omit<ReactTestInstance, 'find' | 'findAllByProps' | 'findAllByType' | 'findByProps' | 'findByType' | 'instance'> |
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.
Can you talk through your thinking in naming this FiberRoot
? I was thinking something along the lines of NativeInstance
or NativeElement
although neither of those are quite right, either.
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 am open to changing this. The FiberRoot
was chosen to make the types consistent with documentation https://www.native-testing-library.com/docs/api-queries#bya11yhint
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 brings up a good point, this change will likely require updating the docs also so that the types match.
timeout?: number | ||
interval?: number | ||
} | ||
export declare function waitForElement<T>(callback: () => T, options?: WaitOptions): Promise<T> |
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.
waitForElement
also take container
as an option.
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 was actually removed, it no longer takes a container 👍
Unrelated to this PR, but the reason for this was that in dom-testing-library
the container is used as the root of a MutationObserver
but we don't actually observe for mutation. It was an option in previous versions, but it didn't actually do anything. This is true for ^1.6.0.
src/index.d.ts
Outdated
// ------------ | ||
|
||
// TODO: options are pretty-format options | ||
export declare function prettyPrint(element: ReactTestRenderer | FiberRoot, maxLength?: number, options?: any): string |
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.
You can import { OptionsReceived } from 'pretty-format'
to get the options.
Also, technically element
can be anything. I would at least make it `ReactTestRenderer | FiberRoot | string
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'm 👍 on this once the native event second parameter is typed (and that type is used for all the fire event functions). Thanks for spending your time on this! It'll be great to get this released - we can iterate on making sure all the types are perfectly matched later.
Great. I'll probably get around to continue work on it on Monday. Also, do you have any suggestions on what should be the type for the NativeEvent second parameter? I left it as any, because I didn't really know what it should be |
NativeEvent is flexible because of handlers like
or:
but it'll also includes any other properties you passed to the constructor on the second arg. So for example:
would give you this on
|
44d2c33
to
f83eacb
Compare
Codecov Report
@@ Coverage Diff @@
## master #5 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 15 15
Lines 290 290
Branches 49 49
=====================================
Hits 290 290 Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #5 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 15 15
Lines 290 290
Branches 49 49
=====================================
Hits 290 290 Continue to review full report at Codecov.
|
Hey! I rebased the PR so that it includes all the changes made over the weekend. I also changed a few things:
|
Thanks again for all of this work. I’m good with getting this in and iterating, like @TAGraves mentioned. Your reasoning makes sense! Next steps for me, I think because of the size of this library it would be a lot more maintainable if these were split into files like in |
🎉 This PR is included in version 1.7.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Closes #2
This PR adds typescript type declarations for the library. The declaration file tries to follow the order of exports from
index.js
.Additionally it tries to fit seamlessly with the current build / release configuration by automatically copying the declaration file to
/dist
after compilation.I would love to get feedback on this and get this as close as possible to the true types.