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

Add unstable_flushSync option #11005

Merged
merged 8 commits into from
Nov 13, 2023
Merged

Add unstable_flushSync option #11005

merged 8 commits into from
Nov 13, 2023

Conversation

brophdawg11
Copy link
Contributor

@brophdawg11 brophdawg11 commented Nov 8, 2023

Closes #11003

Todo:

  • Docs - types and APIs

Copy link

changeset-bot bot commented Nov 8, 2023

🦋 Changeset detected

Latest commit: 3ed602e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@remix-run/router Minor
react-router-dom Minor
react-router Minor
react-router-dom-v5-compat Minor
react-router-native Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@brophdawg11 brophdawg11 linked an issue Nov 8, 2023 that may be closed by this pull request
}

// flushSync + startViewTransition
if (flushSync) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if I could combine these two flows or not code-wise but for the first pass I just kept startViewTransition/flushSync entirely separate from startViewTransition/startTransition. They use the same local states, but the series of sequential effects are solely for startTransition, while flushSync does it all here - bang bang bang

// Interrupting an in-progress transition, cancel and let everything flush
// out, and then kick off a new transition from the interruption state
renderDfd.resolve();
renderDfd && renderDfd.resolve();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be conditional since if we're interrupting a flushSync view transition, we won't have a renderDfd

@@ -514,10 +576,10 @@ export function RouterProvider({
// When we start a view transition, create a Deferred we can use for the
// eventual "completed" render
React.useEffect(() => {
if (vtContext.isTransitioning) {
if (vtContext.isTransitioning && !vtContext.flushSync) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prevents us from ever kicking off the effect chain for flushSync view transitions. No renderDfd means the rest of the effects will no-op

@@ -916,7 +982,6 @@ export interface NavLinkProps
style?:
| React.CSSProperties
| ((props: NavLinkRenderProps) => React.CSSProperties | undefined);
unstable_viewTransition?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a dup - already inherited from LinkProps

@brophdawg11 brophdawg11 merged commit 1193ae5 into dev Nov 13, 2023
3 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/flushsync branch November 13, 2023 21:51
Copy link
Contributor

🤖 Hello there,

We just published version 6.19.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 6.19.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

jonathanpruvost pushed a commit to concordnow/react-router that referenced this pull request Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🗺 Add flushSync option
2 participants