Make PendingNavigation.tsx strict-mode-compatible - #12310
Draft
seyoon20087 wants to merge 3 commits into
Draft
Conversation
We found that PendingNavigation.tsx was NOT strict-mode-compatible out of the box. As `shouldComponentUpdate` (and thus `dispatchLifecycleAction(...)`) is double-invoked in development-mode (when we export StrictMode as default in src/theme/Root.js), this would be a terrible experience during development mode (even though it does not affect production builds). (For example, the progress bar is still displayed at the top even though navigation was finished because that lifecycle method was double-invoked.) To address this problem, we use a field `this.pendingLocation` to track the location that is being preloaded. During the second synchronous render pass (when StrictMode in development), we compare the `nextProps.location` against `this.pendingLocation` and if they match, this returns `false` immediately, which causes the `dispatchLifecycleAction(...)` to not run more than one time. Beyond this fix, this commit also has the added benefit of the following: Checking `if (this.pendingLocation === nextLocation)` inside the asynchronous `.then()` block acts as an **abort-and-debounce guard**. If a user rapidly double-clicks different links (e.g., navigating to Page A and then quickly to Page B), the preloading of Page A will resolve but won't trigger state updates or prematurely (for example) dismiss the progress bar for Page B. * NOTE: parts of these code changes and/or the text of this commit were made using AI Signed-off-by: seyoon20087 <24852454+seyoon20087@users.noreply.github.com>
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Collaborator
How did you find out? We don't have a strict mode setting in Docusaurus (yet). If this is something detected and implemented by AI, please disclose it properly or explain how to reproduce the issues you encountered in a concrete way. Note that refactoring this without asking me first is not a good idea. We are not even sure to keep this component in v4, see #12201 |
Contributor
Author
Sorry about this. First, I created a new site (yarn create docusaurus), and in src/theme/Root.js add these contents: export { StrictMode as default } from "react";with the original 'PendingNavigation.tsx', when add that file, this is the video of the demonstration: Screen.Recording.2026-07-23.at.6.48.26.PM.movA progress bar is displayed even though navigation was completed. (I expected that the progress bar isn't displayed) |
- in the original PendingNavigation component (before I committed to my fork) the component executed side-effects (like 'dispatchLifecycleAction') inside render-phase lifecycles (such as 'shouldComponentUpdate' or 'constructor'). the component also relied on returning 'false' from 'shouldComponentUpdate' to block updates while new route chunks are being preloaded. in strict-mode and/or concurrent mode, render-phase methods might be double-invoked or discarded, with may cause duplicate lifecycle actions or race conditions. also, blocking updates via 'shouldComponentUpdate' while initiating state changes or side-effects breaks React's concurrent rendering model. Further, in the react docs they say that the 'shouldComponentUpdate' method ONLY exists as a performance optimization, not for side-effects (c.f. https://react.dev/reference/react/Component#shouldcomponentupdate-caveats, https://legacy.reactjs.org/blog/2020/02/26/react-v16.13.0.html#notable-bugfixes). - changes: - replaced the 'nextRouteHasLoaded' boolean flag with explicit 'renderedLocation' and 'previousLocation' states. the 'render' method uses 'state.renderedLocation' to keep the current route visible during preloading without blocking react re-renders imperatively. this means we declaratively control what route is displayed on the screen (rather than imperatively doing so using shouldComponentUpdate). - moved initial lifecycle dispatch from 'constructor' (render phase) to 'componentDidMount' (commit phase). - route-preloading and transition-handling related stuff has moved from 'shouldComponentUpdate' to 'componentDidUpdate'. - added cleanup in 'componentWillUnmount' and added unmount checks to prevent possible state updates on unmounted components. Signed-off-by: seyoon20087 <24852454+seyoon20087@users.noreply.github.com>
This check (and any related code inside it) is used only in componentDidMount and componentDidUpdate only. Such methods (and thus code inside 'ExecutionEnvironment.canUseDOM') only ever run the browser, so checks like this is not needed. Signed-off-by: seyoon20087 <24852454+seyoon20087@users.noreply.github.com>
7 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
We found that PendingNavigation.tsx was NOT strict-mode-compatible out of the box.
As
shouldComponentUpdate(and thusdispatchLifecycleAction(...)) is double-invoked in development-mode(when we export StrictMode as default in src/theme/Root.js), this would be a terrible experience during development mode (even though it does not affect production builds).
(For example, the progress bar is still displayed at the top even though navigation was finished because that lifecycle method was double-invoked.)
To address this problem, we use a field
this.pendingLocationto track the location that is being preloaded.During the second synchronous render pass (when StrictMode in development), we compare the
nextProps.locationagainstthis.pendingLocationand if they match, this returnsfalseimmediately, which causes thedispatchLifecycleAction(...)to not run more than one time.Beyond this fix, this commit also has the added benefit of the following:
Checking
if (this.pendingLocation === nextLocation)inside the asynchronous.then()block acts as an abort-and-debounce guard.If a user rapidly double-clicks different links (e.g., navigating to Page A and then quickly to Page B), the preloading of Page A will resolve but won't trigger state updates or prematurely (for example) dismiss the progress bar for Page B.
NOTE: parts of these code changes and/or the text of this commit were made using AI
Pre-flight checklist
Motivation
Test Plan
Test links
Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/
Related issues/PRs
Alternate fix for #10037