vercel / next.js

The React Framework
https://nextjs.org
MIT License
126.85k stars 26.96k forks source link

Router continuously fires `routeChangeError` after using `push` to a dynamic route segment #48839

Open omarestrella opened 1 year ago

omarestrella commented 1 year ago

Verify canary release

Provide environment information

Local environment:
    Operating System:
      Platform: darwin
      Arch: arm64
      Version: Darwin Kernel Version 22.4.0: Mon Mar  6 20:59:28 PST 2023; root:xnu-8796.101.5~3/RELEASE_ARM64_T6000
    Binaries:
      Node: 16.17.0
      npm: 8.15.0
      Yarn: 1.22.17
      pnpm: 7.30.5
    Relevant packages:
      next: 13.2.0
      eslint-config-next: 13.3.0
      react: 18.2.0
      react-dom: 18.2.0

Stackblitz (small recreate):
    Operating System:
      Platform: linux
      Arch: x64
      Version: Ubuntu 20.04.0 LTS Tue Apr 25 2023 15:55:15 GMT-0400 (Eastern Daylight Time)
    Binaries:
      Node: 16.14.2
      npm: 7.17.0
      Yarn: 1.22.19
      pnpm: 8.2.0
    Relevant packages:
      next: 13.2.4
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0

Which area(s) of Next.js are affected? (leave empty if unsure)

Routing (next/router, next/navigation, next/link)

Link to the code that reproduces this issue

https://stackblitz.com/edit/nextjs-btjwni

To Reproduce

  1. You should open your browser console to see the event handlers being called
  2. On the index page, click on the link "Go to /page/[dynamic] with a Router.push"
  3. After landing on the dynamic segment page, click on the link "To Random Dynamic With Router Push »"
  4. Wait a moment (a few seconds) and click on the link "To Random Dynamic With Router Push »" and you will see the router error event, routeChangeError, has fired
  5. Continue to use the same link and see that the event continues to fire each time

Describe the Bug

When you use Router.push in succession on a dynamic route segment, the Router continues to fire the routeChangeError event. This does not happen if you use a <Link /> to perform the page transition.

After you receive the errors from calling the router's push method, you can use a <Link /> to get back into a "good" state. The following Router.push call will not fail, but if you do call Router.push again, it will exhibit the same behavior as before.

At first it seems like the previous Router.push was cancelled, but even if you wait to ensure the promise has resolved, you will still get the error.

Expected Behavior

No router errors occur on successful, non-cancelled, Router.push calls, like

Which browser are you using? (if relevant)

Chrome 112.0.5615.137

How are you deploying your application? (if relevant)

No response

icyJoseph commented 1 year ago

I had initially thought this was because:

This is a funky bug you have created yourself, I think anyway.

There's two preconditions:

  • Shallow routing enabled
  • Pushing onto the same route

If you do those two, even with next/link, you'll awaken the route cancellation, because, you are navigating onto the same page, shallowly, so nothing happens.

And was pretty confident about it because of the usage of Math.random.

However, the bug seems to just rooted on shallow routing. All but the first shallow navigations trigger cancelation. Still investigating why.

That is to say that, next/link, with shallow also behaves as you describe.

Smaller repro

Having a static page: page/static.js:

import React from 'react';

import Link from 'next/link';

export default function Dynamic() {
  return (
    <div>
      <div style={{ marginBottom: '20px' }}>On a static route</div>

      <div>
        <Link href={`/page/static`} shallow>
          Go to Random leet with anchor
        </Link>
      </div>
    </div>
  );
}

Triggers the error by cancelation as well.

icyJoseph commented 1 year ago

I think the error is somehow getting triggered by this part: https://github.com/vercel/next.js/blob/canary/packages/next/src/shared/lib/router/router.ts#L1265-L1276

Not sure about those flags just yet - also wondering if this is really an issue at this point ~

omarestrella commented 1 year ago

I think the error is somehow getting triggered by this part: https://github.com/vercel/next.js/blob/canary/packages/next/src/shared/lib/router/router.ts#L1265-L1276

Yeah, this.clc never seems to clear in the expected way. Sure, its reset in this block, but then this runs: https://github.com/vercel/next.js/blob/canary/packages/next/src/shared/lib/router/router.ts#L1960 and resets the clc function on the router.

Not sure about those flags just yet - also wondering if this is really an issue at this point ~

It's a bit concerning because it makes the error event a bit pointless, or at least not useful for responding/gathering metrics. Is there actually a route error occurring? It doesn't seem that way, the transition to the page works. There is no actual cancellation either, correct? Ultimately, I just can't trust any error the router emits at all at the moment.

icyJoseph commented 1 year ago

You can, just check err.cancelled, those are should be alright to ignore.

omarestrella commented 1 year ago

You can, just check err.cancelled, those are should be alright to ignore.

In my example, I updated the log to read err.cancelled, and it is always true. Which makes sense, the cancellation error is always true: https://github.com/vercel/next.js/blob/canary/packages/next/src/shared/lib/router/router.ts#L85

If this should actually considered a user error, as some unsupported operation (a shallow route transition to the same page) the docs do not seem call that out, so it could just be a documentation concern. It doesn't feel like that should be the case, since the error is fired while Next continues to work, but that could just be expected and undocumented behavior 🤷‍♂️

Thank you for the help!


Edit: Actually, it is as you mentioned, just any shallow change causes the error to fire even though there is no actual error, because this example in the docs: https://nextjs.org/docs/routing/shallow-routing does the same exact thing as described above. So it does seem like this is an actual issue, because the router is really misrepresenting the state (there is no cancellation, you were routed, the query changed, etc etc)

omarestrella commented 1 year ago

To add to all of this, it does seem like quick shallow route changes result the the Router to stop firing the routeChangeComplete events as well. So doing something like:

router.push(`/?counter=${Math.random()}`, undefined, { shallow: true }).then(() => {
  return router.push(`/?counter=${Math.random()}`, undefined, { shallow: true })
}).then(() => {
  return router.push(`/?counter=${Math.random()}`, undefined, { shallow: true })
})

will cause all following shallow routes to fail to fire the routeChangeComplete event even though the route change does occur.

Which can occur in user-space if your users are quickly navigating via controls that call Router.push (quickly paging through a table that changes a query parameter, for example). I can wire up another example, if necessary. This does seem like a deeper issue with shallow routing on the same (static) pages.

Probably related to some other issues, like #46481