nuclear-js
nuclear-js copied to clipboard
optimizely
State updates in ReactMixin are not atomic
Nuclear's current implementation calls reactor.observe separately for each entry in the response to a React component's getDataBindings. If multiple pieces of bound data change in a single dispatch, Nuclear will call setState on the component for each of them and React will dutifully render each time. This means that the exact behavior of your component is dependent on the order in which you define your data bindings (aside from being bad for performance).
The most proximate solution I can see is to call reactor.observe once with all of the keypaths from getDataBindings and call setState once with the complete set of data. This code would be hairy since it has to handle getters as well, but it's doable. I have some concern that it forces more recomputation than necessary but I suspect that's a lesser issue than forcing React to diff repeatedly.
Alternatively, it might make sense to provide a shouldComponentUpdate implementation that returns false for all but the last setState call. I suspect that's the most performant option but I think it would require deeper coupling between ReactMixin and Reactor.
If I understand correctly, batch works to group multiple dispatches. In
this case, a single dispatch is changing multiple pieces of state.
setState is called once for every observed getter or keypath that
changes, rather than once with all changes.
On Nov 16, 2015 11:50 PM, "Ken Carpenter" [email protected] wrote:
See Reactor#batch(fn) in the docs https://optimizely.github.io/nuclear-js/docs/07-api.html. It was added for just this reason.
— Reply to this email directly or view it on GitHub https://github.com/optimizely/nuclear-js/issues/193#issuecomment-157301843 .
@metcalf you are correct in your understanding of the implementation. It would be possible to observe all Getters registered with a component in getDataBindings() and only call setState a maximum of once per dispatch.
I'd be happy to take a PR for this feature.
@metcalf Ooops...misunderstood you there.
I haven't noticed the other behavior you described yet, but I can see how it would be a problem.
@metcalf I understand the situation you've given. But in react's implementation, the number of calls to setState within a short period of time within a component doesn't trigger the same number of time of calls to render function for re-rendering and the diff comparison. There are two things I know so far that's affecting this process. One is react's batching process, the other one is the shouldComponentUpdate function.
According to my experience in Chrome's flame chart for performance tuning and some understanding in the react source, react itself has a batch update process to deal with the changing world around it. setState is the one of the event that they are batching up. So in a single dispatch call, even it's calling setState for many times. it will mostly result in 1 call to the component's re-rendering process.
In order to verify my sayings above, I've done a little experiment myself in my macbook. I've sticked some console logs in shouldComponentUpdate & render within a component that has interests in 5 getters. Plus I make the shouldComponentUpdate always return true for the purpose of digging. When I fired an action that causes 5 getters to update. In my console I only see 1 call each to the functions I'm watching. I will be curious to see what's the situation in your heaviest component in your environment.
Base on the above, I doubt shouldComponentUpdate will have much help in this situation.
@lyonlai thanks for the additional investigation. In my case I was able to identify multiple renders getting triggered for a single dispatch. I'm much less concerned about the performance implications of this than by the potentially unintuitive behavior of having state transitions depend on the order in which data bindings are defined. I agree that shouldComponentUpdate is probably not the right solution. See #195 for a single-observer implementation.
For a bit more context on my specific case: I have a component that contains subcomponents that depends on localization strings stored in Nuclear. It only renders those subcomponents when an "open" flag is set in Nuclear. The "open" flag only returns true if the localization strings are available for rendering.
A single dispatch loads the localizations strings. If the "open" data binding is defined first, the subcomponents attempt to render without the necessary strings and throw an error. If the localization data binding is defined first, everything works properly since the subcomponents receive the strings when they are first rendered.
@metcalf can you build a jsfiddle or similar to show your problem, which will be easier for all of us in this discussion to visualise the problem and find the underlying causes.
I had a look in the implementation. My concern will be the generation of combined getter on the fly will break the current caching mechanism. If you have two component that just happened to watch the same set of getters in data binding, it will calculate the combined getter twice.
See: http://jsfiddle.net/nv6y11d6/4/ (only tested in the latest release channel Chrome).
You can use the badOrder flag at the top of the JS to switch the order in which the data bindings are defined. In one order, it renders a friendly "Hello World" while in the other the dispatch call triggers an uncaught exception.
@metcalf I played around the fiddle and found out the problem is actually quite a common case for our team during development.
So, the root problem here is that the result of a stringGetter is a getter that might be null sometimes , which doesn't have the function you expected in the render function calls then throws an exception. The order of notify is critical in this situation and the solution you provided as combining the getters on the fly does solve this particular situation. But it doesn't solve all of the problems that comes from a getter might results in nullable values.
I have a forked fiddle of your original which illustrates a situation where the combining getters in getDataBindings solution will not work. see http://jsfiddle.net/ejqr6j7x/3/
Similar to what you have before, I only add a second component binds to mystringGetter, which is depending on the value of the stringsGetter. There are two versions of it, a good one and a bad one, controlled by using the badStrings switch on the top, which is similar to badOrder control.
The good one looks like: const mystringGetter2 = [stringsGetter, strings => strings && strings.get('mystring')];
The bad one looks like: const mystringGetter = [stringsGetter, strings => strings.get('mystring')];
I've already set the badOrder to false in this case, which the original app is functioning as you expected. But by turning the badStrings switch on and off. you will see the error.
Nullable value is quite a common thing we have to deal with in our development and I don't think nuclear-js can help much on this one.
We do deal with the fact that the value is nullable. In production, we fire a tracking call to indicate that an expected translation string is missing. This means we would track spurious errors.
By setting state one observer at a time, Nuclear is introducing a new (and invalid) state to my application. Where my store logic only permits two states ("not open, no strings" and "open, has string"), Nuclear is effectively introducing a third intermediate state ("open, no strings"). Every specific case will have plausible workarounds but having Nuclear silently redefine my state machine is unintuitive, error prone and difficult to debug. On Nov 18, 2015 3:10 AM, "Yun Lai" [email protected] wrote:
@metcalf https://github.com/metcalf I played around the fiddle and found out the problem is actually quite a common case for our team during development.
So, the root problem here is that the result of a stringGetter is a getter that might be null sometimes , which doesn't have the function you expected in the render function calls then throws an exception. The order of notify is critical in this situation and the solution you provided as combining the getters on the fly does solve this particular situation. But it doesn't solve all of the problems that comes from a getter might results in nullable values.
I have a forked fiddle of your original which illustrates a situation where the combining getters in getDataBindings solution will not work. see http://jsfiddle.net/ejqr6j7x/3/
Similar to what you have before, I only add a second component binds to mystringGetter, which is depending on the value of the stringsGetter. There are two versions of it, a good one and a bad one, controlled by using the badStrings switch on the top, which is similar to badOrder control.
The good one looks like: const mystringGetter2 = [stringsGetter, strings => strings && strings.get('mystring')];
The bad one looks like: const mystringGetter = [stringsGetter, strings => strings.get('mystring')];
I've already set the badOrder to false in this case, which the original app is functioning as you expected. But by turning the badStrings switch on and off. you will see the error.
Nullable value is quite a common thing we have to deal with in our development and I don't think nuclear-js can help much on this one.
— Reply to this email directly or view it on GitHub https://github.com/optimizely/nuclear-js/issues/193#issuecomment-157680841 .
Hm this issue seems a bit deeper than I first imagined. React shouldn't have any performance hits if you are calling setState more than once in a single tick as it batches the actual DOM updates on nextTick.
I'll have to investigate further if this is a desired behavior as the default ReactMixin (which could possibly break backward-compat in edge cases) or if this should be a separate mixin.
Someone else on the team just got bitten by this. In the most recent case, we pass an error code and descriptive message to the UI. We decide what to render based on the machine-readable code and then assert that there's a message available to display if the state requires it.
Nuclear can introduce several invalid state transitions where the message is null but the code requires it. We could of course update the view code to silently swallow the invalid state, but this hinders our ability to assert that we're displaying good UI in production.
:+1: Just ran into this myself. Would be a great to not have to worry about the order in which keys are defined in getDataBindings. Hoping @metcalf's PR can resolve this
