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

Router Error Events in Shallow Routing by Skipping cancelHandler Creation #61772

Closed
rvetere opened this issue Feb 7, 2024 · 2 comments · Fixed by #61771
Closed

Router Error Events in Shallow Routing by Skipping cancelHandler Creation #61772

rvetere opened this issue Feb 7, 2024 · 2 comments · Fixed by #61771
Labels
bug Issue was opened via the bug report template. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.

Comments

@rvetere
Copy link
Contributor

rvetere commented Feb 7, 2024

Link to the code that reproduces this issue

https://codesandbox.io/p/devbox/elastic-christian-3yqgtv

import { useRouter } from "next/router";
import { useEffect, useRef } from "react";

export default function Home() {
  const counter = useRef(0);
  const router = useRouter();

  useEffect(() => {
    const handleRouteChangeError = (err: any) => {
      console.error("Route change error", err);
    };
    router.events.on("routeChangeError", handleRouteChangeError);
    return () => {
      router.events.off("routeChangeError", handleRouteChangeError);
    };
  });

  const handleLoadMore = () => {
    counter.current++;
    router.replace(`/?take=${counter.current}`, undefined, { shallow: true });
  };
  return (
    <div>
      Click "Load More" at least two times to see the error(s)
      <button onClick={handleLoadMore}>Load More</button>
    </div>
  );
}

To Reproduce

  1. Open the codesandbox example and start clicking "Load More"
  2. After the first click, it starts firing "Route Cancelled" errors in the console

Current vs. Expected behavior

  1. Open the codesandbox example and start clicking "Load More"
  2. It never fires a "Route Cancelled" error

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 23.2.0: Wed Nov 15 21:53:18 PST 2023; root:xnu-10002.61.3~2/RELEASE_ARM64_T6000
Binaries:
  Node: 20.11.0
  npm: 10.2.4
  Yarn: 4.0.2
  pnpm: 8.15.1
Relevant Packages:
  next: 14.1.0
  eslint-config-next: N/A
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.3.2
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

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

Which stage(s) are affected? (Select all that apply)

next dev (local), next build (local), next start (local)

Additional context

Problem

We've identified a bug within Next.js's pages router when utilizing the shallow routing API, specifically when invoking router.replace with the { shallow: true } option, like so:

router.replace('/?counter=10', undefined, { shallow: true });

Shallow routing is designed to update the URL without running data fetching methods such as getServerSideProps, getStaticProps, or getInitialProps. However, a side effect of this process is that it skips the clean-up of the cancelHandler. This leads to router error events being fired erroneously, causing confusion and potential stability issues, as the system behaves as if an error occurred when, in fact, none did.

Solution

This PR addresses the issue by modifying the shallow routing logic to also skip the creation of the cancelHandler. Given that shallow routing operations are synchronous and do not involve data fetching or other asynchronous tasks that might need to be canceled, the cancelHandler is unnecessary in this context.

We have created a PR which would fix this behavior: #61771

@rvetere rvetere added the bug Issue was opened via the bug report template. label Feb 7, 2024
@github-actions github-actions bot added the Navigation Related to Next.js linking (e.g., <Link>) and navigation. label Feb 7, 2024
@rvetere
Copy link
Contributor Author

rvetere commented Feb 15, 2024

added test-case

(wow, i had NO idea how sophisticated the whole e2e setup is of next.js - AMAZING!)

shuding added a commit that referenced this issue Feb 28, 2024
…Creation (#61771)

### Problem

We've identified a bug within Next.js's pages router when utilizing the
shallow routing API, specifically when invoking `router.push` with the
`{ shallow: true }` option, like so:
 
```javascript
router.push('/?counter=10', undefined, { shallow: true });
```

Shallow routing is designed to update the URL without running data
fetching methods such as getServerSideProps, getStaticProps, or
getInitialProps. However, a side effect of this process is that it skips
the clean-up of the cancelHandler. This leads to router error events
being fired erroneously, causing confusion and potential stability
issues, as the system behaves as if an error occurred when, in fact,
none did.
 
### Solution

This PR addresses the issue by modifying the shallow routing logic to
also skip the creation of the cancelHandler. Given that shallow routing
operations are synchronous and do not involve data fetching or other
asynchronous tasks that might need to be canceled, the cancelHandler is
unnecessary in this context.

fixes #61772

---------

Co-authored-by: Shu Ding <g@shud.in>
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant