add $F as a force property to the component instance

[xnimorz]

Objective

This PR adds $F instance field which is in charge of the force updating component. It makes forceUpdate work predictable

Closes Issue

It closes Issue #1534

[coveralls]

Coverage increased (+0.006%) to 95.133% when pulling 76fafc2 on xnimorz:master into ff5245f on infernojs:master.

[trueadm]

Why is this change needed?

[xnimorz]

No need, reverted it

[trueadm]

Why is this line needed?

[xnimorz]

As $F stores the information whether changes should be forced or not, we assume that they are not forced by default unless forceUpdate is called. The default value of $F is false.

If forceUpdate is called we switch $F to true, and wait before it will be time for our component to render. After the component updates, we want to return the default value for the component. So, it's why I'm using $F = false.

And finally, the component potentially could plan further changes via setState or even forceUpdate during its updating cycle. It means we want to set up all defaults before the component starts its updating cycle. So I cache value in const force.

[Havunen]

I think there is larger issue here. So what should happen if there are 2 setState calls in queue, then one forceUpdate call is made. now after this change SCU will not be called for those 2 setStates in the queue, is this expected behavior?

[xnimorz]

@Havunen At the moment if we have 2 (or more) pending setState the pending state changes will be merged here: https://github.com/infernojs/inferno/blob/master/packages/inferno/src/core/component.ts#L26-L28
Then only 1 update will be planned: https://github.com/infernojs/inferno/blob/master/packages/inferno/src/core/component.ts#L41-L43

Talking about forceUpdate once it called it's clear, that the developer wants to force the update of the component for some reason (maybe to force layout or like in my example in the issue). So we don't need to divide renders into "normal" flow and "force" in that case (as in the end the component should be re-rendered with the latest changes). So having 2 updates cycles wouldn't help us but has perf downgrade.

[Havunen]

Yes however there has been many bugs reported in the past that some callback (SCU, didUpdate or similar) is not called in some particular case and their application does not work same way as it does with react. I have seen people write code where they call setState X times and they are expecting same number of calls to component (did/will) update so this change is breaking change for those applications if they use forceUpdate.. :/

[Havunen]

I think to merge this we would at minimum need to add test cases for these cases and have the expected result so that its compatible with React implementation

[xnimorz]

Yes, it seems like a breaking change, as when a developer calls forceUpdate they expect that the component will be updated. But at the moment we have a possible situation when it doesn't work in such a way.

Also, If I'm not mistaken about the difference between Inferno and React, that react doesn't merge all pending states to 1 update (e.g. React merge them if they are called inside react event handler or lifecycle method, but doesn't merge if the changes are planned in the setTimeout). Inferno plans all changes as a part of the QUEUE and merges them if the component is presented in the QUEUE.

I'll add a test case

[Havunen]

Also, If I'm not mistaken about the difference between Inferno and React, that react doesn't merge all pending states to 1 update (e.g. React merge them if they are called inside react event handler or lifecycle method, but doesn't merge if the changes are planned in the setTimeout).

React behavior is not consistent in all cases, it depends how setState is called. If its called from event handlers it works diferently...

Calling setState from event handlers ( type into the text input and check console log )
https://jsfiddle.net/pnwLh7au/

When setstate call origin is not event handler it does it different way see ( open url and check console log ):
https://jsfiddle.net/hbspta0y/

[xnimorz]

e.g. React merge them if they are called inside react event handler or lifecycle method, but doesn't merge if the changes are planned in the setTimeout

As I said, and you example is also about it: React merges changes if they are called inside react event handler or lifecycle method.
But if you call changes inside setTimeout it wouldn't work in a such way: https://jsfiddle.net/egd1kuz6/

BTW your examples work in the same way during the update, but in the first external call yes, the second example has different behavior, I agree

Your first example: only 1 render during updating cycle which was called because of event handler:
https://jsfiddle.net/pnwLh7au/

Your second example: 3 re-renders during init, as each setTimeout and forceUpdate makes different component update. However, during updating cycle we have only 1 re-render:
https://jsfiddle.net/hbspta0y/

My example with timeouts: https://jsfiddle.net/egd1kuz6/
In this case we get out from event handler, make another macro-task, which makes our component to update each time setTimeout or force update is called:

However, Inferno already has a different flow: it updates only 1 time per component as all the updates are stored in the QUEUE and we check if there are any of the components in the QUEUE. So, I think it's consistent behavior and my changes only support it.

[xnimorz]

Just pushed tests @Havunen 76fafc2

[Havunen]

Ok, thanks for tests LGTM :)

[xnimorz]

Thanks for merging @Havunen !