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

[Failing tests] startTransition invalidates downstream memoizations #17284

Closed

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Nov 6, 2019

Like dispatch or setState, we should return the same startTransition function on every render. This prevents it from invalidating downstream memoizations, like useCallback.

We do use useCallback internally so that a new startTransition is only created when the config object changes. However, if you pass an object, this does nothing because the object different every time.

Potential fixes:

  • Instead of detecting changes in the config object, compare the fields (timeoutMs et al) directly. This fixes the worst case (inline config object) but still means we'll occasionally re-create startTransition functions.
  • Track the current config object on an internal mutable field using an effect and read from that instead of binding a new method each time. This requires extra work in the commit phase, but combined with the previous option (comparing the fields instead of the entire config object) that should be rare.
  • Instead of passing the config object to the useTransition hook, pass it to the startTransition function. This avoids needing to bind the config object to the method. Slightly less ergonomic, however, we're planning to change the default config object so that it's not needed in most cases. So it's only in special cases that you'd actually use one. This is the most likely option.

Like `dispatch` or `setState`, we should return the same
`startTransition` function on every render.
@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 6, 2019

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 35280c5:

Sandbox Source
naughty-leakey-9mjzl Configuration

@sizebot
Copy link

sizebot commented Nov 6, 2019

Warnings
⚠️ Base commit is broken: 0f3838a

Size changes (experimental)

Generated by 🚫 dangerJS against 35280c5

@sizebot
Copy link

sizebot commented Nov 6, 2019

Warnings
⚠️ Base commit is broken: 0f3838a

Size changes (stable)

Generated by 🚫 dangerJS against 35280c5

@necolas necolas added the React Core Team Opened by a member of the React Core Team label Jan 8, 2020
@acdlite
Copy link
Collaborator Author

acdlite commented Aug 28, 2020

Closed by #19719

@acdlite acdlite closed this Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants