-
Notifications
You must be signed in to change notification settings - Fork 26.7k
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
Conversation
@timneutkens Any feedback on this approach? Or a suggested alternative? Would really like to see this feature move through. |
Yes this feature would be really awesome! Thanks @desaias for your PR! |
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 |
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Could you add a test for this? |
@timneutkens I've added a test to the |
@timneutkens is there anything else needed to be able to get this into the 7.0.3 release? |
@desaias is there anything |
@rauchg 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 #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 |
You can already prevent routing the 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> |
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 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. |
@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? |
@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 And you decide that for a certain page, you most certainly want to intercept the ability to show that dialog, for any I certainly don't think so. It introduces a lot of indirection, it makes the behavior of |
I'm closing the issue based on the two responses. If I got something wrong, please let me know |
@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. |
@timneutkens @rauchg You might have overlooked #2476 while you were closing issues and moving on |
@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. |
What happened here? Is it not possible to intercept with beforeRouteChangeStart ? |
why this was closed? |
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. |
Can we add this feature? It will be very useful. What's the actual alternative to do that ? |
Also need this feature, please reopen |
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. |
@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. |
@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. |
+1 for this feature. I have the same usecase that warn the user about the unsaved change before navigating the other pages. |
+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. |
+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. |
+1 for this feature. I have the same usecase that warn the user about the unsaved change before navigating the other pages. |
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. |
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. |
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 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 |
+1 for this feature.... :( |
Opened a discussion about this: #12348 |
@rauchg @timneutkens Any updates on this issue? This is obviously highly desired by the community. |
Would also like to see this feature implemented :) |
I need it :) |
Same! Looks like it's close? |
Hello everybody! Greetings, Flo. |
Added
beforeRouteChangeStart
to router so that you can stop the router from changing if the callback function returns false.fixes #2694