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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type definitions for the proposed React Hooks API #30057

Merged
merged 13 commits into from Nov 9, 2018

Conversation

Projects
None yet
@Kovensky
Contributor

Kovensky commented Oct 26, 2018

I had free time so I went and wrote these 馃槆

Based on the code at facebook/react#13968

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

@Kovensky Kovensky requested a review from johnnyreilly as a code owner Oct 26, 2018

@Kovensky Kovensky force-pushed the Kovensky:react-hooks branch from e4febc5 to 93345f3 Oct 26, 2018

@typescript-bot typescript-bot added this to Waiting for Reviewers in Pull Request Status Board Oct 26, 2018

@typescript-bot

This comment has been minimized.

typescript-bot commented Oct 26, 2018

@Kovensky Thank you for submitting this PR!

馃敂 @johnnyreilly @bbenezech @pzavolinsky @digiguru @ericanderson @morcerf @tkrotoff @DovydasNavickas @onigoetz @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Needs Author Attention in Pull Request Status Board Oct 26, 2018

@typescript-bot

This comment has been minimized.

typescript-bot commented Oct 26, 2018

@Kovensky 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!

@epeli

This comment has been minimized.

Contributor

epeli commented Oct 26, 2018

There's also #30036

@Kovensky

This comment has been minimized.

Contributor

Kovensky commented Oct 26, 2018

Ah, I didn't pay enough attention 馃槼

I'll check that out

@johnnyreilly

This comment has been minimized.

Member

johnnyreilly commented Oct 31, 2018

Good work @Kovensky !

@FredyC

This comment has been minimized.

Contributor

FredyC commented Oct 31, 2018

Um, why is this (testing only)? Apparently, the quality of these has already surpassed the other PR #30036 and the author is not responding to comments at all.

@typescript-bot typescript-bot moved this from Needs Author Attention to Check and Merge in Pull Request Status Board Oct 31, 2018

@typescript-bot

This comment has been minimized.

typescript-bot commented Oct 31, 2018

A definition owner has approved this PR 猸愶笍. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

*
* @see https://reactjs.org/docs/hooks-reference.html#usecontext
*/
function useContext<T>(context: Context<T> | Consumer<T> /*, (not public API) observedBits?: number|boolean */): T;

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Oct 31, 2018

Collaborator

Since these are all proposed APIs, how would you feel about marking them as @experimental or @alpha?

This comment has been minimized.

@FredyC

FredyC Oct 31, 2018

Contributor

Please remove Consumer from there. Even though it works, for now, it shows a warning and shouldn't be used like that.

This comment has been minimized.

@Kovensky

Kovensky Nov 1, 2018

Contributor

I'm going to use @version experimental since it's documented in usejsdoc.org

// Unlike redux, the actions _can_ be anything
type Reducer<S, A> = (prevState: S, action: A) => S;
// The identity check is done with the SameValue algorithm (Object.is), which is stricter than ===
type IdentityCheckInputList = ReadonlyArray<any>;

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Oct 31, 2018

Collaborator

ReadonlyArray<any> is shorter and it might be better to avoid the indirection of thinking of another type.

This comment has been minimized.

@Kovensky

Kovensky Nov 1, 2018

Contributor

I renamed it to have a shorter name and to use unknown

// await to properly engage Suspense.
type EffectCallback = () => (void | (() => void));
interface MutableRefObject<T> {

This comment has been minimized.

@DanielRosenwasser

DanielRosenwasser Oct 31, 2018

Collaborator

Why isn't RefObject the right abstraction here?

This comment has been minimized.

@FredyC

FredyC Oct 31, 2018

Contributor

See this #30036 (comment).

@DanielRosenwasser

This comment has been minimized.

Collaborator

DanielRosenwasser commented Oct 31, 2018

Sending over my tests at Kovensky#1

@DanielRosenwasser DanielRosenwasser referenced this pull request Oct 31, 2018

Closed

React Hooks #30036

@typescript-bot typescript-bot moved this from Check and Merge to Needs Author Attention in Pull Request Status Board Oct 31, 2018

Kovensky added some commits Nov 1, 2018

Fix the factory in useImperativeMethods affecting the T parameter
Adds a second type parameter to prevent TypeScript from just refining T
to {} if the result of the callback is a supertype of the ref's type.
Return immutable ref object from useRef<T>(T|null) to prevent misuse
If you need the ref to be mutable just pass <T|null> as the generic argument.

@Kovensky Kovensky force-pushed the Kovensky:react-hooks branch from 9ff61c3 to 12c9045 Nov 9, 2018

* @see https://reactjs.org/docs/hooks-reference.html#usecallback
*/
// TODO (TypeScript 3.0): <T extends (...args: never[]) => unknown>
function useCallback<T extends (...args: any[]) => any>(callback: T, inputs: InputIdentityList): T;

This comment has been minimized.

@Kovensky

Kovensky Nov 9, 2018

Contributor

There is an inference issue with useCallback that creates silent implicit anys, but I could not find any way to work around this. Even completely removing the extends clause still creates silent anys.

<button
  onClick={React.useCallback(e => {
    // this works...
    e.preventDefault()
    // but e is actually any!
    e.nonsense
  })}
/>

I suppose one of the generic inference candidates for "function with one argument" happens to pass and the return type inference never gets to be used. Also, because it is generic, that makes the type of the first argument "actually be" (e: any) => void which silences the implicit any check.

@typescript-bot typescript-bot moved this from Needs Author Attention to Check and Merge in Pull Request Status Board Nov 9, 2018

@johnnyreilly johnnyreilly merged commit a161857 into DefinitelyTyped:master Nov 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@johnnyreilly

This comment has been minimized.

Member

johnnyreilly commented Nov 9, 2018

Well done @Kovensky!

@typescript-bot typescript-bot removed this from Check and Merge in Pull Request Status Board Nov 9, 2018

@rhysforyou

This comment has been minimized.

Contributor

rhysforyou commented Nov 9, 2018

Get to see this merged! I can鈥檛 wait to move my hooks demo projects over to TS 馃槉

@FredyC FredyC referenced this pull request Nov 9, 2018

Closed

React - more type safety for componentDidCatch #30068

2 of 9 tasks complete
@juergenzimmermann

This comment has been minimized.

juergenzimmermann commented Nov 9, 2018

@Kovensky Thank you! I just used it and found only 1 issue: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L696 should be type EffectCallback = () => (void | (() => void) | Promise<void>); instead of type EffectCallback = () => (void | (() => void)); for async functions.
Do you see a chance to get this fixed?

@Kovensky

This comment has been minimized.

Contributor

Kovensky commented Nov 9, 2018

I specifically wrote in a comment that it's not allowed to be async. The currently published build doesn't warn but the React team has a PR that will warn if the result is not void | (() => void) and will specifically tell you to not use an async function if the result object has a .then function.

To get the same (unsafe) result, invoke an async IIFE inside your sync useEffect callback.

PS: "why is this unsafe?" -- it has always been unsafe to have async lifecycles because there's the possibility that they attempt to mutate state after the Component has unmounted.

@kube

This comment has been minimized.

kube commented Nov 9, 2018

馃檶

@juergenzimmermann

This comment has been minimized.

juergenzimmermann commented Nov 9, 2018

@Kovensky @kube Perhaps you can have a short look at my current problem which yields an error. What is best practice?

async function load() {
    const response = await findById(id);  // await (and async above) from TypeScript
    if (response !== undefined) {
        setBook(response); // setter for the state
    }
}
useEffect(load, [id]);
@FredyC

This comment has been minimized.

Contributor

FredyC commented Nov 9, 2018

@juergenzimmermann That's exactly what you cannot do :) The async/await returns a Promise.

@Kovensky

This comment has been minimized.

Contributor

Kovensky commented Nov 9, 2018

What you can do to bypass the issue, both with types and the future React warning is to pass () => { load() } to useEffect instead (the brackets are important to prevent the implicit return).

@kube

This comment has been minimized.

kube commented Nov 9, 2018

@Kovensky @DanielRosenwasser
First, thanks for your work.

I'd like to understand why did useRef was reverted to immutable property, is it only for compatibility with current definitions?
Then, how should the following be written?

const Timer = () => {
  const tickInterval = useRef<number>(null)

  useEffect(() => {
    tickInterval.current = setInterval(tickFunction, 420) // ERROR: current is read-only
    return () => clearInterval(tickInterval.current!)
  }, [])
}
@Kovensky

This comment has been minimized.

Contributor

Kovensky commented Nov 9, 2018

@kube I wrote it in the JSDoc. If you want it to be mutable, make the generic argument be <number|null>.

I made it like this to avoid misuse while making DOM/Component refs more convenient.

If your ref is intended to be used as a DOM/Component ref, you should not be mutating current, React will. As those refs always start with null, I made it so a ref that would not declare it includes null but is initialized with null anyway be an immutable ref. For all other cases, you have to include all acceptable values in the type argument, including the initial value.

@kube

This comment has been minimized.

kube commented Nov 9, 2018

Indeed. Thanks.

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