Conversation
|
@theKashey @obweger What do you think of this way of moving forward? |
|
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)?) |
|
Sorry for long response, missed this PR. 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) |
src/store/create-state.js
Outdated
| // so multiple actions affecting the same store gets combined | ||
| schedule(storeState.notify); | ||
| if (defaults.unstable_concurrent && unstable_concurrent !== false) { | ||
| startTransition(storeState.notify); |
There was a problem hiding this comment.
I am not sure you are "allowed" to startTransition from within library. This should be user-space controlled.
a41232c to
f82876a
Compare
f82876a to
236d9fc
Compare
|
Sorry if it took so long. I have now published To recap: In Jira we will play around with |
|
Hmmm... with current implementation of 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, turns out |
But it's wrapped in a scheduler here |
I did test with / without wrapping in scheduler.
Without scheduler:
|
|
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.
|
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
startTransitionand 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_concurrentto test the performance of dropping our custom scheduler.So, by default this PR only fixes the warning that
defaults.batchUpdates = falsehas on React 17+.For Jira, we can play around with
defaults.unstable_concurrent = true | false | undefined, where:undefinedis the current container implementation + uSES + batching (if enabled)falseis the new container implementation + uSES + batching (if enabled)trueis startTransition + no uSES + no batching