nodejs.org icon indicating copy to clipboard operation
nodejs.org copied to clipboard

feat: frozen router

Open ovflowd opened this issue 10 months ago • 13 comments

Description

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

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 https://github.com/nodejs/nodejs.org/issues/6409

ovflowd avatar Apr 26 '24 11:04 ovflowd

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

vercel[bot] avatar Apr 26 '24 11:04 vercel[bot]

cc @nodejs/nodejs-website

ovflowd avatar Apr 26 '24 11:04 ovflowd

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 🔗

github-actions[bot] avatar Apr 26 '24 11:04 github-actions[bot]

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 :zzz: 0 :x: 0 :fire: 5.735s :stopwatch:

github-actions[bot] avatar Apr 26 '24 11:04 github-actions[bot]

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

ovflowd avatar Apr 26 '24 11:04 ovflowd

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 avatar Apr 26 '24 11:04 ovflowd

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.

ovflowd avatar Apr 27 '24 15:04 ovflowd

@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?

abizek avatar Apr 28 '24 14:04 abizek

@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 avatar Apr 28 '24 14:04 ovflowd

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

ovflowd avatar Apr 29 '24 12:04 ovflowd

(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

abizek avatar May 02 '24 17:05 abizek

(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!

ovflowd avatar May 03 '24 18:05 ovflowd

@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 😬.

abizek avatar May 04 '24 03:05 abizek

@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 avatar May 11 '24 19:05 ovflowd

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

ovflowd avatar May 11 '24 19:05 ovflowd

No problem :) This is the PR - https://github.com/nodejs/nodejs.org/pull/6709

abizek avatar May 12 '24 04:05 abizek

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. 🙃

abizek avatar May 16 '24 07:05 abizek

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.

ovflowd avatar May 16 '24 21:05 ovflowd