Prevented Link Double Render in React #79
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
Previously the Links were rendering twice. Once when they render and then again because the onNavigation listener fired. This only happened on initial navigation (not refresh) because that's synchronous - subsequent React renders are async so the onNavigate fires before the render and React only renders once. The Back Link still renders twice because it's possible that navigating the same Url can change the Back Links if there's a custom truncate url that allows repeated states
The lazy update shouldn't know about the internal state of components. Think Back Link needs different state anyway, so better to use forceUpdate. The onNavigate listener isn't added if lazy so no need to track state for a lazy update
The Back Link doesn't need to re-render if the crumb hasn't changed. But, back link might have active props in future and these could change even if crumb hasn't (could navigate to same url as crumb but the crumb trail could still have that crumb in it if there's custom truncation). Including the stateContext url in the check caters for this and matches with Refresh and Navigation Links
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.
The forceUpdate in the onNavigate handler caused an unnecessary re-render. Immediately after a Link component renders the onNavigate handler fires which causes a re-render even though nothing's changed.
ReactDOM.render is synchronous only if mounting a brand new component. When ReactDOM.render is async then the unnecessary re-render doesn't happen because the onNavigate fires before the first render and so React squashes the two render requests into one.
Changed the Link components to store the state context url in React state each time they render. Then the onNavigate handler only re-renders if the the state context url has changed since previous render.
The Back Link stores the crumb as well as the url in state because the crumb can change even when the context url doesn't (with a custom truncate crumb trail). Accessing the crumb is cheap because it's not evaluated each time, it's just take from the cached crumbs.
Didn't implement this in knockout or angular because the Links update so often it didn't seem worth it.