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

Added beforeRouteChangeStart #5377

Closed
wants to merge 3 commits into from
Closed

Added beforeRouteChangeStart #5377

wants to merge 3 commits into from

Conversation

desaias
Copy link

@desaias desaias commented Oct 3, 2018

Added beforeRouteChangeStart to router so that you can stop the router from changing if the callback function returns false.

fixes #2694

@nicktate
Copy link

nicktate commented Oct 9, 2018

@timneutkens Any feedback on this approach? Or a suggested alternative? Would really like to see this feature move through.

@DonovanCharpin
Copy link

Yes this feature would be really awesome! Thanks @desaias for your PR!

@jgoux
Copy link

jgoux commented Oct 12, 2018

I think this would also enable cool things like defining custom animated transitions based on the next route!

I'm experimenting with this idea right now using react-router : https://codesandbox.io/s/1y3on612j4

@nicktate
Copy link

Bump @timneutkens again for any feedback on this PR.

@@ -38,6 +38,7 @@ export default class Router {
this.subscriptions = new Set()
this.componentLoadCancel = null
this._beforePopState = () => true
this._beforeRouteChangeStart = () => true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this implementation, why can't we just use the eventemitter?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to prevent the the routing from continuing in the async change method. This is analogous to what beforePopState is doing but for general route changes.

@timneutkens
Copy link
Member

Could you add a test for this?

@desaias
Copy link
Author

desaias commented Oct 25, 2018

@timneutkens I've added a test to the with-router integration test and a snippet to the readme

@desaias
Copy link
Author

desaias commented Oct 31, 2018

@timneutkens is there anything else needed to be able to get this into the 7.0.3 release?

@rauchg
Copy link
Member

rauchg commented Nov 1, 2018

@desaias is there anything beforeRouteChangeStart enables that can't be done with regular APIs? e.g.: by wrapping <Link> with your own component

@desaias
Copy link
Author

desaias commented Nov 2, 2018

@rauchg beforeRouteChangeStart is essentially the same as what beforePopState does when you are clicking the browser back/forward buttons except it is for when you click a next/link within your application.

I don't believe there is another way to short circuit the router from changing, even if you were to create a custom component that wraps <Link>. But I could me misunderstanding what you are suggesting.

#2236 and #2694 were used as inspiration/feedback for this solution, along with testing other potential solutions.

This seemed like the least obtrusive method since it always returns true unless you specifically set beforeRouteChangeStart.

@timneutkens
Copy link
Member

timneutkens commented Nov 2, 2018

You can already prevent routing the onClick way without a global side-effect though:

https://github.com/zeit/next.js/blob/canary/packages/next-server/lib/link.js#L125-L132

function confirm(event) {
  if(!window.confirm('Should we route?')) {
    event.preventDefault()
  }
}

<Link href="/about">
  <a onClick={confirm}>About page</a>
</Link>

@DonovanCharpin
Copy link

It's not really the same use case here. Let's say I have an HOC and this HOC check if the child form on the page is dirty. If so, then asks the user if he wants to save his changes before leaving the page. The only way to manage this is to be able to get the event before the route change starts and return true/false regarding the user choice.

There is actually no way to subscribe/unsubscribe to this router event with nextjs. For big apps, we cannot add a method on every Link child because this Link doesn't know what is happening in the page and why the route shouldn't change, it's just a menu loaded by another component, accessible by the user.

In my case, the only way would be to share the form state (redux-form) in every component containing a Link and manage another redux state to know if the links should or should not trigger a user dialog. If we could subscribe to this event and manage if the route should change, it would really improve the developer experience and allow us to implement more controls.

@desaias
Copy link
Author

desaias commented Nov 2, 2018

@timneutkens Your example seems fine for one page with one link, but as soon as you start building out an application with navigation components, multiple pages and multiple components, I don't see how adding an onClick to every potential link would scale.

If you wanted to do some logic before changing routes on Page A, but not on Page B, that both share the same navigation component, how would you prevent that logic from leaking out to other pages?

@rauchg
Copy link
Member

rauchg commented Nov 2, 2018

@DonovanCharpin @desaias what I'm hearing here is: we want to make it easier, not possible.

As a thought experiment, imagine that instead of navigating away, you have a <Button> component that upon clicked shows a dialog.

And you decide that for a certain page, you most certainly want to intercept the ability to show that dialog, for any <Button> on the page. Do you think it would be acceptable to define a Button.onBeforeShow global handler, just to make this use-case easier?

I certainly don't think so. It introduces a lot of indirection, it makes the behavior of <Link> contingent on "global magical state". If I as a Next.js developer instead see <AppLink>, then I know it can have different behavior instead, and I read the code to understand it.

@rauchg rauchg closed this Nov 2, 2018
@rauchg
Copy link
Member

rauchg commented Nov 2, 2018

I'm closing the issue based on the two responses. If I got something wrong, please let me know

@desaias
Copy link
Author

desaias commented Nov 3, 2018

@rauchg it is impossible to cancel/intercept a route change once it has started, which react-router supports. Unless I'm missing something and it somehow possible, please point me in the right direction since I can't seem to figure that out and I'm pretty sure others have the same issue.

@desaias
Copy link
Author

desaias commented Nov 3, 2018

@timneutkens @rauchg You might have overlooked #2476 while you were closing issues and moving on :shipit:

@akotlar
Copy link

akotlar commented Nov 18, 2018

@rauchg That's a frankly perplexing stance to take. By it we should have no functionality whatsoever that modifies state of a scope that is greater than the immediate block, because can always be constructed in a confusing way. You're already defining "global" state: a singleton router that modifies the entire state of the program, and may be triggered from any other part of the application.

As others have pointed out, it's a standard feature of other packages (Angular, AngularJS, React Router), which means the underlying action is useful, and that people adopting NextJS will be unpleasantly surprised by its omission, especially given the difficulty in using an alternative router w/ next. If the functionality already exists, please help us find it. If not, some version of this pull request should be accepted.

@spoeken
Copy link

spoeken commented Feb 28, 2019

What happened here? Is it not possible to intercept with beforeRouteChangeStart ?

@mitramejia
Copy link

why this was closed?

@Wildhoney
Copy link

Another use case @rauchg is the ability to listen for users pressing the esc to cancel navigation. It works by default for server navigation, but not for client navigation unless you provide a way to cancel a route change.

@michelmoretti
Copy link

Can we add this feature? It will be very useful. What's the actual alternative to do that ?

@todesstoss
Copy link

Also need this feature, please reopen

@sultanassi95
Copy link

I am 99.99% closing a migration of a 3 years old React application to Next.js

It took me long of effort and a long time to do that.

The only missing piece is directly connected to this conversation!

Preventing the use from moving away with a very specific customized dialogue/modal, this case was supported by React-Router, I feel disappointed that I can't yet figure a way to doing that and finishing what I started 80 days ago!

I can never agree with @rauchg on what he said.

@Timer
Copy link
Member

Timer commented Jul 1, 2019

@sultanassi95 most all use cases can be covered without blocking navigation globally -- if you shared your app on Spectrum (or share the relevant code here), we could probably help you come up with simpler a solution.

@sultanassi95
Copy link

@Timer I have covered that case doing a draft save for the user, which saves the data that a user didn't submit, but the actual problem is with what my client wants.

Regarding that, it still a very good thing to include in Next, hope you get it in your schedule, I have seen this requested almost two years ago with promises from the team that this will be considered.
Thanks for replying back. ✌

@raymondsze
Copy link

+1 for this feature. I have the same usecase that warn the user about the unsaved change before navigating the other pages.

@vendramini
Copy link

+1 for this feature. I'm building a PWA with Onsen UI, it has its own navigation and I need to intercept the routing from next, use this information with all query params to properly change Onsen's navigation.

@lcorz
Copy link

lcorz commented Aug 23, 2019

+1 for this feature. As a navigation component, repeated clicks will cause the page to be loaded repeatedly, even though it is already in the corresponding page. I can't intercept the route jump by judging the path.

@ryuoryuo
Copy link

+1 for this feature. I have the same usecase that warn the user about the unsaved change before navigating the other pages.

@vitorrd
Copy link

vitorrd commented Sep 15, 2019

@DonovanCharpin @desaias what I'm hearing here is: we want to make it easier, not possible.

As a thought experiment, imagine that instead of navigating away, you have a <Button> component that upon clicked shows a dialog.

And you decide that for a certain page, you most certainly want to intercept the ability to show that dialog, for any <Button> on the page. Do you think it would be acceptable to define a Button.onBeforeShow global handler, just to make this use-case easier?

I certainly don't think so. It introduces a lot of indirection, it makes the behavior of <Link> contingent on "global magical state". If I as a Next.js developer instead see <AppLink>, then I know it can have different behavior instead, and I read the code to understand it.

Correct me if I'm wrong, but what you're hearing is your user base telling you a core feature is much harder to implement on your framework than it should be, and being arrogant about it won't get you farther.

This pull request works, introduces minimal change and since you're replacing the entire "browser router" with your own thing, I don't think saying it makes anything contingent on "global magical state" applies, since that's exactly what every router, ever, does - including the browser's built-in one.

@desaias Thanks for your work on this. I've locked my Next.js version and applied your patch. I hope your request is checked by someone interested in improving this framework.

@ptomasroos
Copy link
Contributor

I have some feedback on this subject as I bumped into trying to hold of the transition from happening.

The reason why I would like to cancel in beforeRouteChangeStart rather than on my onClick handler is that, sometimes if an animation hasn't completed I would like to delay the navigation for xx-xxx ms.

In the case where I prevent this on onClick there is a noticeable delay because, then I need to fetch js for the page, run the data fetching, compared to if it was possible in beforeRouteChangeStart. The page is already fetched and initial props, which means that very likely I do not need to cancel this route because it came much later in the life cycle.

@okonet
Copy link
Contributor

okonet commented Oct 22, 2019

Sorry for bumping this one but I also agree that the "indirection" argument doesn't work when it comes to the app's state since it's a global state already.

Now, in order to implement something that could be a clean code we need to do this in our own Link component:

return href.startsWith("http") || href.startsWith("mailto:") ? (
    <>
      <Component
        as="a"
        textStyle="link"
        href={`${href}${query ? "?" + queryString.stringify(query) : ""}`}
        title={`Visit ${href}`}
        {...rest}
      >
        {children}
      </Component>
    </>
  ) : (
    <Component
      as="a"
      textStyle={isActive ? "active" : "link"}
      onClick={async e => {
        e.preventDefault();

        if (pageContentChanged) {
          const confirmedLeave = await confirm(
            "Are you sure you want to leave the page without saving first?"
          );

          if (!confirmedLeave) {
            return;
          }
        }

        router.push(routeToString({ href, query }));
      }}
      {...rest}
    >
      {children}
    </Component>
  );

and then set pageContentChanged via context somewhere on in the app.

@sfatali
Copy link

sfatali commented Mar 13, 2020

+1 for this feature.... :(

@toreyhickman toreyhickman mentioned this pull request Apr 11, 2020
1 task
@janhesters
Copy link

Opened a discussion about this: #12348

@JStein92
Copy link

@rauchg @timneutkens Any updates on this issue? This is obviously highly desired by the community.

@Emiliano-Bucci
Copy link

Would also like to see this feature implemented :)

@kahirokunn
Copy link

I need it :)

@harrisosserman
Copy link

Same! Looks like it's close?

@Flo0806
Copy link

Flo0806 commented May 27, 2021

Hello everybody!
I don't understand one thing ... A clean approach to a solution has been shown and has already been confirmed (in several) discussions that the framework does not offer a clean way to stop navigation (browser button, router). Only workarounds that can throw an error can help.
So why is this topic on hold? It has already been mentioned several times that it would be an important function.
Thanks for the feedback in advance @rauchg @timneutkens !

Greetings, Flo.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Browser Back Button does not trigger window.onbeforeunload