react

react copied to clipboard

Describe the bug As the doc, NavList can be used with Next.js. But it occur a Error: Maximum update depth exceeded.

Error: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.

<NavList>
  <Link href="/" passHref>
    <NavList.Item aria-current="page">
      Home
    </NavList.Item>
  </Link>
  <Link href="/about" passHref>
  <NavList.Item>About</NavList.Item>
  </Link>
  <Link href="/contact" passHref>
    <NavList.Item>Contact</NavList.Item>
  </Link>
</NavList>

To Reproduce Steps to reproduce the behavior:

1. Use NavList with Next.js' Link
2. Open the page

I reproduced it in my repo.

Expected behavior Show NavList with Link correctly.

Screenshots

Desktop (please complete the following information):

• OS: macOS
• Browser: chrome
• Version 103

Hi, thanks for reporting. We'll have a look!

Thanks for including a repro! 💙

Hi there 👋🏻 Just wanted to let you know that we've noted this issue but haven't prioritized it yet. We'll revisit again soon.

Hi! I took a look at this bug because it seemed interesting and after a few days of exploration I was able to figure out the reason why this is happening. So the whole flow goes like:

NextJS's Link component stores the ref internally and inside the callback ref, it uses the element to keep track of whether the element is visible or not here on line 414.

So when the LinkItem component receives the above callback _ref via forwardRef, it forwards it down to the Link component that we are rendering via _PrivateItemWrapper.

Once the ref is applied on our Link component, the next Link component re-renders (because of the useIntersection) and it forces our LinkItem component to re-render but because of this re-render the reference of the _PrivateItemWrapper has now changed and primer's Link component re-mounts which causes the ref to be set again which again sets up the intersection observer which then re-renders and we are stuck in this infinite cycle.

I stopped passing the forwardedRef from LinkItem to Link and this error didn't happen so I am pretty sure that the root cause is that the reference of _PrivateItemWrapper is not stable but I am not sure how I should proceed with making it a stable reference. I tried to wrap it inside callback but the sx prop is not stable (coming from NavList.Item) and hence the callback is also not able to provide the stable reference.

I also thought of moving it outside as a component in its own but it needs props of LinkItem to pass it to the Link component and I am not sure how to achieve that so that seems to be a dead end as well.

Would really love if @siddharthkp / @colebemis anyone of you can help me with the direction I'd really love to land some contributions as part of Hacktoberfest 22!

Thanks!

Thank you for doing some further investigation here, @MihirGH! I'll move this back to our team's inbox to see if anyone has capacity to weigh in on your suggested approach this month.

@lesliecdubs pinging you since it's a team capacity question

Hello again 👋🏻 We unfortunately don't have the capacity to address this directly at the moment. I'm going to move this into our backlog, which we review and reprioritize each quarter of the financial year.

• New context for reprioritisation: https://github.com/github/accessibility/discussions/4089

@lesliecdubs this was placed in the backlog previously, and has come back on our radar - is this something we should prioritize this quarter?

@tallys from a priority standpoint, refer to this comment (GitHub staff only).

That said, this seems like a good candidate for assignment a little later this quarter after the Primer Engineering team has gotten further along with the next major release and is more focused on accessibility work. I'll move this back to "unprioritized" for now, but took an action item to pull this back into the "Inbox" for triage again around August 21.

Agree that this is a good candidate, we will continue to triage and assess the right time to put this on our roadmap.

@joshblack is going to take a look, confirm sizing this week, and revisit from there.

Just wanted to share a small update, was looking into this but am running into a hydration issue with Next.js 🤔 Looking into what is causing that and also looking into the underlying issue here based on the comments above 👀

Finally was able to revisit this (so sorry about the delay 🤦) but it seems like the example is incorrect as it leads to nested <a> tags which was causing the validation error and hydration mismatch 🤔

Revisiting the example, I believe something like this would be more accurate and would not run into the problems mentioned above:

import {NavList} from '@primer/react'
import {useRouter} from 'next/router'
import Link from 'next/link'
import React from 'react'

function NavItem({href, children}) {
  const router = useRouter()
  const isCurrent = typeof href === 'string' ? router.asPath === href : router.pathname === href.pathname
  return (
    <NavList.Item as={Link} href={href} aria-current={isCurrent ? 'page' : false}>
      {children}
    </NavList.Item>
  )
}

export default function IndexPage() {
  return (
    <NavList>
      <NavItem href="/">Dashboard</NavItem>
      <NavItem href="/pulls">Pull requests</NavItem>
      <NavItem href="/issues">Issues</NavItem>
      <NavItem
        href={{
          pathname: '/[owner]/[repo]',
          query: {owner: 'test', repo: 'test'},
        }}
      >
        Summary
      </NavItem>
    </NavList>
  )
}

Curious what folks here in the thread think 🤔