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

Adding time-slicing with startTransition to prevent hydration from blocking the main thread for too long for those users who immediately scroll. #4313

Merged

Conversation

sanjaiyan-dev
Copy link
Contributor

This PR is heavily inspired from tweet by Ryan Florence.

I am extremely sorry if I made any mistakes :(

@changeset-bot
Copy link

changeset-bot bot commented Aug 14, 2022

⚠️ No Changeset found

Latest commit: 7d22519

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added pkg: react Related to React (scope) pkg: integration Related to any renderer integration (scope) labels Aug 14, 2022
@matthewp
Copy link
Contributor

Thanks @sanjaiyan-dev! Astro has client directives to control timing for components, so if you wanted to hydrate on idle you would do: <Counter client:idle />. So that part of this PR I don't think we would want. However, the startTransition part we might. Can you explain what it is that that does, and how that helps with scrolling?

@sanjaiyan-dev
Copy link
Contributor Author

Thanks @sanjaiyan-dev! Astro has client directives to control timing for components, so if you wanted to hydrate on idle you would do: <Counter client:idle />. So that part of this PR I don't think we would want. However, the startTransition part we might. Can you explain what it is that that does, and how that helps with scrolling?

Sorry, I have removed the requestIdleCallback from the code 🙌

@sanjaiyan-dev
Copy link
Contributor Author

sanjaiyan-dev commented Aug 16, 2022

Thanks @sanjaiyan-dev! Astro has client directives to control timing for components, so if you wanted to hydrate on idle you would do: <Counter client:idle />. So that part of this PR I don't think we would want. However, the startTransition part we might. Can you explain what it is that that does, and how that helps with scrolling?

According to Ryan Florence this would improve the load performance of the webpage.

And by making hydrating without blocking main thread would enhance the performance too.


My thoughts 💭 (might be misleading)

Astro js support multiple JavaScript Frameworks 🤩 at the time and if any of the component doing some work which is high priority/heavy we can interrupt the hydration of react component for sleeker user experience 💪🙌

@matthewp
Copy link
Contributor

Thanks @sanjaiyan-dev. Can you explain exactly how startTransition improves performance? Is it something you want to do all of the time? I'm hesitant to make a change based on a claim in a tweet and would like to understand more specifics.

If startTransition were something you wanted to do all of the time why wouldn't it just be built into render(). This makes me think there is nuance here and it's possible that somethings startTransition is a good idea and sometimes it is not, so I'd like to understand that nuance before we decide on making the change or not.

@sanjaiyan-dev
Copy link
Contributor Author

Thanks @sanjaiyan-dev. Can you explain exactly how startTransition improves performance? Is it something you want to do all of the time? I'm hesitant to make a change based on a claim in a tweet and would like to understand more specifics.

If startTransition were something you wanted to do all of the time why wouldn't it just be built into render(). This makes me think there is nuance here and it's possible that somethings startTransition is a good idea and sometimes it is not, so I'd like to understand that nuance before we decide on making the change or not.

Reference from React js Docs
Updates in a transition yield to more urgent updates such as clicks.

Updates in a transition will not show a fallback for re-suspended content. This allows the user to continue interacting with the current content while rendering the update.


Reference from tweet by React js team
Transitions are interruptible, don’t block user input, and keep your app more responsive

@sanjaiyan-dev
Copy link
Contributor Author

Thanks @sanjaiyan-dev. Can you explain exactly how startTransition improves performance? Is it something you want to do all of the time? I'm hesitant to make a change based on a claim in a tweet and would like to understand more specifics.
If startTransition were something you wanted to do all of the time why wouldn't it just be built into render(). This makes me think there is nuance here and it's possible that somethings startTransition is a good idea and sometimes it is not, so I'd like to understand that nuance before we decide on making the change or not.

Reference from React js Docs Updates in a transition yield to more urgent updates such as clicks.

Updates in a transition will not show a fallback for re-suspended content. This allows the user to continue interacting with the current content while rendering the update.

Reference from tweet by React js team Transitions are interruptible, don’t block user input, and keep your app more responsive

Time slicing is the thing that keeps me firmly in the React camp. I've built so much UI in my career where user interactions can cause rapid UI updates.

Without time slicing, the main thread gets blocked and you get jank.

React 18 just solves--and hides--the problem completely.

https://twitter.com/ryanflorence/status/1547961298813497355?s=20&t=BxmSnZmZfN4nOyTVw5aOTQ

@matthewp
Copy link
Contributor

I asked the question here: https://twitter.com/matthewcp/status/1562057134967328769

@matthewp matthewp added the semver: minor Change triggers a `minor` release label Aug 24, 2022
@matthewp
Copy link
Contributor

This will go out with the next minor release, thanks!

@matthewp matthewp merged commit 3b5f34d into withastro:main Aug 25, 2022
@sanjaiyan-dev
Copy link
Contributor Author

This will go out with the next minor release, thanks!

Tks 💪🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope) pkg: react Related to React (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants