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

Draft: added typescript types to waitfor functions #982

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lindapaiste
Copy link

What:

Converted 5 files from .js to .ts. All files relate the waitFor functions.

Why:

This PR continues the conversion of the codebase to TypeScript - issue #494

How:

  • Added type annotations to function arguments and return values.
  • Used type assertions where the TS type is too broad to access the desired property, for example (error as Error).name and (node as unknown) as Screen).debug.
  • Broadened existing types from Element or HTMLElement to Node in a few places in order to match the implementation, which shows that the container could be a Document as well.
  • Typed the node argument in the getWindowFromNode function as an object that maybe has properties of a Window, Document, or Element. This allows access to these properties without as assertion.
  • Created interface for timerApi object to avoid TS errors when adding additional properties after the object has been created.

Checklist:

  • Documentation added to the
    docs site
  • Tests
  • Typescript definitions updated
  • Ready to be merged

Issues

I'm considering this a draft because I still have lingering questions where I'm not sure of the desired behavior.

My biggest confusion is the setTimeout / clearTimeout functions. The type for a timeout is a number in the DOM and a NodeJS.Timeout object in node. Which one is it at runtime? Can it be either, or can it be typed cleanly as just one? The inferred type from using the global clearTimeout variable is that it's either a function which takes a number or a function that takes a NodeJS.Timeout object. This creates an impossible function which cannot be called, as we don't know which of the two arguments it takes. I've had to circumvent it with a nasty type assertion that allows the function to be called with either type.

(clearTimeout as (t: NodeJS.Timeout | number) => void)(timer)

I'm happy to explain the logic behind any of these changes.

There's a lot more assertions then I would normally make if I was writing this code from scratch. I generally prefer type guard functions, which refine the type of the variable when true. For example the isPromise function which I put in the wait-for.ts file would be better placed in utilities file where it can be exported and used in multiple places. Then this mess in helpers.ts

if (((node as unknown) as Promise<unknown>).then instanceof Function)

can be replaced with

if (isPromise(node))

Is that acceptable and/or desirable? To me it seems like a no-brainer but I am trying to limit the scope of my changes as much as possible.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2e2f9d6:

Sandbox Source
react-testing-library-examples Configuration

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.

None yet

1 participant