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_concurrent behaviour #221

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

albertogasparin
Copy link
Collaborator

@albertogasparin albertogasparin commented Jan 5, 2024

After the long conversation in #219 , this PR tries to ratify the behaviour.

First of all, let's be clear on the end state: there is no batching and no scheduling done by RSS. We use startTransition and only provide concurrent opt-out via global settings or per-store.
Given we need time to investigate the impact (and Atlassian is not ready for that anyway), we will use defaults.unstable_concurrent to test the performance of dropping our custom scheduler.

So, by default this PR only fixes the warning that defaults.batchUpdates = false has on React 17+.

For Jira, we can play around with defaults.unstable_concurrent = true | false | undefined, where:

  • undefined is the current container implementation + uSES + batching (if enabled)
  • false is the new container implementation + uSES + batching (if enabled)
  • true is startTransition + no uSES + no batching

@albertogasparin
Copy link
Collaborator Author

@theKashey @obweger What do you think of this way of moving forward?
@obweger Could you test if defaults.unstable_concurrent = false works for you (leaving batchUpdates false too) ? That should buy us time to understand the impact of defaults.unstable_concurrent = true and how much stuff could break with it

@obweger
Copy link
Contributor

obweger commented Jan 18, 2024

Very happy to try @albertogasparin! What's the best way to consume this version? (I suppose you don't plan to officially release it (yet)?)

@theKashey
Copy link

Sorry for long response, missed this PR.
I think that disabling batching and delegate everything to R18 auto-batching is a good solution we should aim for.

Jira is not ready for it as there are few places relying on current flow of events, but I think we should just correct them (but that would take time)

// so multiple actions affecting the same store gets combined
schedule(storeState.notify);
if (defaults.unstable_concurrent && unstable_concurrent !== false) {
startTransition(storeState.notify);

Choose a reason for hiding this comment

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

I am not sure you are "allowed" to startTransition from within library. This should be user-space controlled.

@albertogasparin
Copy link
Collaborator Author

Sorry if it took so long. I have now published [email protected].

To recap:
@obweger you should probably set defaults.batchUpdates = false and defaults.unstable_concurrent = false to get the previous behaviour.

In Jira we will play around with defaults.unstable_concurrent and see what happens perf wise.

@albertogasparin
Copy link
Collaborator Author

albertogasparin commented Mar 17, 2024

Hmmm... with current implementation of defaults.batchUpdates = false and defaults.unstable_concurrent = ... the result not great, as I see a lot of warnings in Jira about bad setState (as we remove the scheduling).

I've tried other implementations but I always end up with warnings as soon as we try sync updates. I feel like that (sync container events) is a pattern we cannot support on R17+ anymore without having react complain 😞
Also removing scheduling makes Jira feel more unresponsive (larger, sync re-renders) so it is not an easy situation. I feel like we might need everyone on R18 with createRoot to really decide what is best and which options to provide.

Also, turns out unstable_batchedUpdates is a noop in React 18 ( facebook/react#24831 (comment) ) , however unclear if both with/without create root.

@theKashey
Copy link

turns out unstable_batchedUpdates is a noop in React 18

But it's wrapped in a scheduler here

@albertogasparin
Copy link
Collaborator Author

But it's wrapped in a scheduler here

I did test with / without wrapping in scheduler.
With scheduler:

  • 🟢 React does not complain because effects go on next tick
  • 🟠 long tasks are broken into multiple (marginally longer due to paint) renders
  • 🔴 children end up receiving the update later

Without scheduler:

  • 🟢 children receive the update immediately
  • 🔴 React complains that we update state somewhere while rendering something else
  • 🔴 we create large single long tasks (unless we use startTransition)

@theKashey
Copy link

Are the last two points are 🔴 or 🟠 ? How bad it is? Do we need to improve anything here?

This is something I don't really understand in React 18 / concurrent mode - it embrases tearing and glitches throwing away "single source of truth" and other stuff.

While this might work "ok-ish" with prop drilling or Context API where update propagation is "controlled", with "external stores" this causes only tearing and 🤷‍♂️ HOTs as some components are doing what they should not.

useTransition is totally fine to defer rendering and the scheduler is a good solution to manage UI updates, but some states can only be correct or not correct, and cannot afford to be incorrect.

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.

3 participants