Data: Schedule render by store update via setState #7746
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aduth
added
[Type] Bug
tofumatt
approved these changes
Jul 6, 2018
packages/data/src/index.js
Outdated
| shouldComponentUpdate( nextProps, nextState ) { | ||
| const hasPropsChanged = ! isShallowEqual( this.props, nextProps ); | ||
|
|
||
| // Only render if result of props change or merge props update |
There was a problem hiding this comment.
I think this comment is worded a bit strangely so I'm going to tweak it.
packages/data/src/index.js
Outdated
| const nextMergeProps = getNextMergeProps( nextProps ); | ||
| if ( ! isShallowEqual( this.mergeProps, nextMergeProps ) ) { | ||
| // While side effects are typically discouraged in this | ||
| // lifecycle methods, this component is very much on a hot |
There was a problem hiding this comment.
"hot code path" is a great turn of phrase but it might confuse ESL readers (or just folks unfamiliar with that colloquium). I'm just gonna tweak this comment.
|
|
|
My line hast been breached! |
|
My tabs are set to 2-width in JS because everyone else in the universe uses 2-spaces for JS. If it should be four then we should set it in the |
|
Oh come now, it's a pretty well-accepted objective truth that tabs are far superior than spaces. |
|
💥 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7722
This pull request seeks to resolve an issue with
withSelectstate derivations where a subscribe update could be called with old versions ofthis.propsduring a scheduled update. Its subsequentforceUpdatewould thereby prevent the defaultshouldComponentUpdatefrom taking place, where new props would be expected to be accounted for. The revised implementation here usessetStatewith awareness of the state change by referential equality condition to allow a props change to be accounted for after the subscribe.Implementation notes:
I have thus far been unsuccessful in recreating this via a failing unit test. It's unclear if the render reconciliation flow is different in Enzyme /
react-test-renderer. Neither have shown promise. The general idea would be that a prop update from a parent would be applied after a child had already triggeredforceUpdate, and this prop update should but does not have an impact on thegetDerivedStateFromProps. In the case of #7722, this prop islastBlockUID, where the presence of a previous default block should be preventing the appender from being shown.The specific handling in React where
shouldComponentUpdateis being skipped occurs here:https://github.com/facebook/react/blob/4fe6eec15bc69737910e5c6c749ebc6187d67be0/packages/react-reconciler/src/ReactFiberClassComponent.js#L816-L825
I had explored a number of alternative approaches here, none of which proved to provide stability in ensuring (a) synchronous application of state changes (otherwise resulting in strange behaviors like caret positioning shifting) or (b) major performance regressions.
See: https://gist.github.com/aduth/0b5b6ed26acbbf2f6387cb46d42059fe
Testing instructions:
Repeat Steps to Reproduce from #7722, verifying that no ghost appender appears.
Ensure unit tests pass:
Verify there are no performance regressions by typing within the Demo post.