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

feat: frozen router #6675

Closed
wants to merge 1 commit into from
Closed

feat: frozen router #6675

wants to merge 1 commit into from

Conversation

ovflowd
Copy link
Member

@ovflowd ovflowd commented Apr 26, 2024

Description

This PR fixes the navigation issues we've been facing due to Next Router behavior (a long-term bug as explained on vercel/next.js#49279, the solution is based on vercel/next.js#49279 (comment))

I've also fixed the React Props of the HexagonGrid whose where flakey.

Validation

Navigate between pages, and you'll see that the components are not unmounting and the navigation to be seamless.

Related Issues

Closes #6409

@ovflowd ovflowd requested a review from a team as a code owner April 26, 2024 11:36
Copy link

vercel bot commented Apr 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Apr 26, 2024 11:36am

@ovflowd ovflowd added the github_actions:pull-request Trigger Pull Request Checks label Apr 26, 2024
@ovflowd
Copy link
Member Author

ovflowd commented Apr 26, 2024

cc @nodejs/nodejs-website

Copy link

github-actions bot commented Apr 26, 2024

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 98 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 96 🟢 98 🟢 100 🟢 92 🔗
/en/download 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 99 🟢 100 🟢 100 🟢 92 🔗

Copy link

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 91%
90.04% (588/653) 76.08% (175/230) 92.18% (118/128)

Unit Test Report

Tests Skipped Failures Errors Time
128 0 💤 0 ❌ 0 🔥 5.735s ⏱️

@ovflowd ovflowd changed the title feast: frozen router feat: frozen router Apr 26, 2024
@ovflowd
Copy link
Member Author

ovflowd commented Apr 26, 2024

Oh, I didn't notice but using the frozen router, all the pages get broken and the content remains the same 🤦

@ovflowd
Copy link
Member Author

ovflowd commented Apr 26, 2024

Hey @abizek if you want to try to keep working on your initial proposal but try something from this approach. Unfortunately, is a well-known Next.js bug that the router will cause all the layouts to remount on the App router, except if we're nesting layouts (https://nextjs.org/docs/app/building-your-application/routing/layouts-and-templates#nesting-layouts) which is not possible on our situation as we use a catch-all segment.

@styfle @leerob I'd super appreciate some help here 🙇

@ovflowd
Copy link
Member Author

ovflowd commented Apr 27, 2024

I tried spending some considerate amount of time today by transforming our logic to support Nested Layouts, but even by using Nested Layouts the nested layouts get remounted... Only the Root Layout doesn't.

@abizek
Copy link
Contributor

abizek commented Apr 28, 2024

@ovflowd I tried nested layouts and I did not find the nested layouts to be rerendering, but I quickly ran into issues with the client-server context. I also tried moving withLayout into the root layout file but the context issue is also there. No luck with frozen router too. We need the client-server context based on the latest pathname but also avoid triggering rerender. Any suggestions?

@ovflowd
Copy link
Member Author

ovflowd commented Apr 28, 2024

@ovflowd I tried nested layouts and I did not find the nested layouts to be rerendering, but I quickly ran into issues with the client-server context. I also tried moving withLayout into the root layout file but the context issue is also there. No luck with frozen router too. We need the client-server context based on the latest pathname but also avoid triggering rerender. Any suggestions?

I was able to make nested layouts work flawlessly locally, (had to make some changes to the rendering engine) but it worker nicely and with clean code.

But in the end the current issue is a Next.js issue. I've let folks at Vercel aware of this, and there are already a bunch of bug reports.

So let's leave it as it is for the time being.

@ovflowd ovflowd marked this pull request as draft April 29, 2024 11:23
@ovflowd
Copy link
Member Author

ovflowd commented Apr 29, 2024

As there's no much we can do at the moment, I'll keep this as a draft.

@abizek
Copy link
Contributor

abizek commented May 2, 2024

(Not trying to plug in my PR)
Since the issue is still there, it seems like a temporary workaround would be nice. What do you think? @ovflowd

@ovflowd
Copy link
Member Author

ovflowd commented May 3, 2024

(Not trying to plug in my PR) Since the issue is still there, it seems like a temporary workaround would be nice. What do you think? @ovflowd

I believe yes, I have the feeling we can do a simple "change" on the client-side. Since the BaseLayout is kept tidy, can you create a new React Provider (for the client side)

Something like NavigationStateProvider, for the time being the state would be an object of this shape something like:

Record<string, { x: number, y: number }>

Which can store element-based navigation state, so it can be used like this:

// assume this is the ProgressionBar Component

// version 1, returns a ref
const ref = useNavigationState('progressionSidebar');

// or version 2, you pass a ref
const ref = useRef<HTMLDivElement>(null);

useNavigationState('progressionSidebar', ref);

Within the hook, the ref is attached to an "onScroll" event (aka passive event listener), which then every time it scrolls (please add a debouncer) it updates the state from the NavigationProvider for that entry.

The idea here is once the hook is mounted again (because the component gets remounted during navigation), we see that the value of the current scroll position is different from the one stored, and we do a scroll event for that element)

ie:

// assume this is inside the hook

useEffect(() => {
  if (ref.y !== state.y) {
    // update position
  }

// no effect dependencies since only on mount
}, []);

I believe this would be a good workaround for the time being! LMK what you think!

abizek added a commit to abizek/nodejs.org that referenced this pull request May 4, 2024
Temporary workaround for nodejs#6409
Get more info here - nodejs#6675

Signed-off-by: abizek <[email protected]>
@abizek
Copy link
Contributor

abizek commented May 4, 2024

@ovflowd I think that makes sense for a temp workaround. I made a draft PR with the changes. But I have some questions on it 😬.

@ovflowd
Copy link
Member Author

ovflowd commented May 11, 2024

@ovflowd I think that makes sense for a temp workaround. I made a draft PR with the changes. But I have some questions on it 😬.

Can you link me to the PR? 👀

@ovflowd ovflowd closed this May 11, 2024
@ovflowd
Copy link
Member Author

ovflowd commented May 11, 2024

Also sorry for the delayed reply, @abizek I was OOO 🙇

@ovflowd ovflowd deleted the feat/frozen-router branch May 11, 2024 19:53
@abizek
Copy link
Contributor

abizek commented May 12, 2024

No problem :) This is the PR - #6709

@abizek
Copy link
Contributor

abizek commented May 16, 2024

I can't stop wondering how the nextjs team solved this issue on their website. I can see that the sidebar on the docs page does rerender. There is no sidebar on the showcase or blog pages. But I can't find more info as the docs builder is closed source. 🙃

@ovflowd
Copy link
Member Author

ovflowd commented May 16, 2024

I can't stop wondering how the nextjs team solved this issue on their website. I can see that the sidebar on the docs page does rerender. There is no sidebar on the showcase or blog pages. But I can't find more info as the docs builder is closed source. 🙃

That is utmost interesting.

github-merge-queue bot pushed a commit that referenced this pull request May 18, 2024
* feat: Navigation state provider and hook

Temporary workaround for #6409
Get more info here - #6675

Signed-off-by: abizek <[email protected]>

* refactor: Navigation state hook effect

Signed-off-by: abizek <[email protected]>

* docs: Add comment explaining eslint-disable-next-line

Signed-off-by: abizek <[email protected]>

---------

Signed-off-by: abizek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Learn" pages sidebar scrolls back to top on every page change
2 participants