Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Flickering page on route navigation #3767

Conversation

callingmedic911
Copy link
Member

@callingmedic911 callingmedic911 commented Nov 20, 2021

Closes #2124.

Why was this happening?
PageLoader was getting unmounted on each route change. And when it remounts it switches to the default state - page is undefined. The cause of unmounting is from activeRouteTree (screenshot 1) in router.tsx. It swaps out the Route, so the whole subtree is unmounted.

image

This PR is the possible fix that updates the LocationAwareRouter to maintain a single route element and copy all attributes/props from the active route. So, react can, well, react.

Things to consider now:

  • Is this a good approach to fix this? EDIT: Tests are failing, clearly I missed something. Will look into it. EDIT2: Using different approach now.
  • Do we need to copy over anything else apart from props, children or other properties of activeRoute element?
  • Finalize approach for testing.

@jtoar
Copy link
Contributor

jtoar commented Nov 21, 2021

Awesome find @callingmedic911! The beta React docs talk a lot about the role tree structure plays in React's rendering/committing: https://beta.reactjs.org/learn/preserving-and-resetting-state. The new docs are really great.

image

A simple test for this may be checking that the tree structure stays exactly the same between renders (except for the Page at the very end).

@callingmedic911
Copy link
Member Author

Seems like the fix won't be straightforward like this because activeChildren is not always a Route. It could be n-level nested with Set.

Checking how we can improve here.

@callingmedic911
Copy link
Member Author

callingmedic911 commented Nov 21, 2021

Trying this new approach, where unmount still happens but now the Router context holds current page module, so it instead of undefined it resets old module. So, at least white flash doesn't appear.

Long term fix can be a bit of refactoring. Theoretically, we have to put Loader outside of Route/activeChildren and pass download page module as a prop to the current Route. Then instead of returning PageLoader from InternalRoute, it can return Page directly. But this seemed too much code change considering we are very near to v1. However, if we agree to it I can proceed with guidance from you guys - @Tobbe @jtoar.

@callingmedic911 callingmedic911 force-pushed the fix/flashing-blank-page-on-route-navigation branch from 92374e9 to 72e7ddd Compare November 21, 2021 19:32
@Tobbe
Copy link
Member

Tobbe commented Nov 22, 2021

@callingmedic911 Please have a look at this commit and see what you think of my alternative solution for this issue Commit #2 for PR #3772

@callingmedic911
Copy link
Member Author

Closing in favour of #3772.

@redwoodjs-bot redwoodjs-bot bot removed this from the next-release-priority milestone Nov 26, 2021
@callingmedic911 callingmedic911 deleted the fix/flashing-blank-page-on-route-navigation branch November 26, 2021 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigating to a route not previously visited flashes white
4 participants