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

Wrap router actions in startTransition #1096

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

hugo-vrijswijk
Copy link

@hugo-vrijswijk hugo-vrijswijk commented Jul 24, 2024

This wraps the router state actions in startTransition. This allows router updates to be non-blocking. React recommends all router updates to be wrapped in a transition.

I.e. if you click on 1 page, you can immediately navigate to another page without waiting for the first page to finish rendering.

I've wrapped 2 actions in startTransition, these made the most sense to me, but I'm not sure if both of them are necessary. The code is a bit complex 😅

@rpiaggio
Copy link
Collaborator

Thank you very much. It would be great to get this working.

I don't think the second action is necessary since it doesn't update (React) state, only the url. Things should work fine with just the first action, the one wrapping $.setState.

Also, I'm a bit worried about the F.transSync. In theory, it could introduce an async boundary, and React won't mark state updates as transitions in async code. It makes it explicit that the wrapped function must be synchronous. But I think this can be solved by turning things around and using F.transSync(startTranstition($.setState(...))).

Finally, I'm not sure startTransition works with class compnents' setState, at least I can find no evidence of it. It seems to require a setState from a useState hook. Could you confirm if this actually works? Maybe you have already tested it or can test it on an example with the same functionality as https://react.dev/reference/react/useTransition#examples ?

@hugo-vrijswijk
Copy link
Author

hugo-vrijswijk commented Jul 25, 2024

I've tried it out, making something similar to the examples. It works as expected with only the first startTransition around the setState, so I've removed the second one now

@rpiaggio
Copy link
Collaborator

Awesome! Thank you very much! Would you mind moving the startTransition inside the transSync ?

@hugo-vrijswijk
Copy link
Author

Awesome! Thank you very much! Would you mind moving the startTransition inside the transSync ?

Yes, done

Copy link
Collaborator

@rpiaggio rpiaggio left a comment

Choose a reason for hiding this comment

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

LGTM

@rpiaggio rpiaggio merged commit a1aa1bb into japgolly:topic/react18 Jul 25, 2024
2 checks passed
@hugo-vrijswijk hugo-vrijswijk deleted the router-transition branch July 25, 2024 14:08
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.

2 participants