Returning `false` in a before filter still changes window.location

[Valloric]

As per docs, returning false in a matching before filter should stop the navigation. Matching routes are not executed, but the URL in the location bar is still changed.

Looking at the source code of the handleRequest function, the only thing that happens when a request is halted is that a requestHalted event is sent. Since handleRequest is called when Davis.location.onChange is triggered, the URL location change should be "undone" in some way.

[arxpoetica]

I'm glad this issue already exists, as I just discovered it. Is there a short-term fix? I would call this a pretty significant bug. If I have to, I'll do a pull request, but I'm totally swamped for time at the moment.

[Valloric]

There's a short-term fix, which is monkey-patching Davis. Here's an example (this works):

function customDavisEventHandler(event) {
  if (shouldStopNavigation()) {
    event.stopImmediatePropagation();
    event.preventDefault();
    return false;
  }
}
Davis.$(document).delegate(app.settings.formSelector,
                           'submit',
                           customDavisEventHandler);

Davis.$(document).delegate(app.settings.linkSelector,
                           'click',
                           customDavisEventHandler);

app.start();

First, notice how all this patching has to be done before the call to app.start()!

You use this in place of with returning false in the before handler. This code adds a custom event handler that runs before the Davis's handlers; if the shoulStopNavigation function (which you have to provide) returns true, then the Davis handlers that trigger Davis's processing don't execute.

[olivernn]

I'm not sure automatically rolling back to the previous route if a filter fails is something that Davis should do, it seems to me that it is something fairly app specific. What is your use case for this feature? It might be something that I haven't thought off.

I think there is a simpler solution to rolling back to the previous location using the events that the app fires:

var autoRollback = function () {
  var lastRequestPath = window.location.pathname

  this.bind('runRoute', function (req) {
    lastRequestPath = req.location()
  })

  this.bind('requestHalted', function (req) {
    Davis.history.replace(new Davis.Request (lastRequestPath), { silent: true })
  })
}

The above is a small plugin that stores a reference to the previous location, and when a request is halted replaces the current state with the previous state, just use it in your app like this:

var app = Davis(function () {
    this.use(autoRollback)

    // your routes etc here…
})

Whilst building this plugin I came accross a bug in Davis that when fixed should make this a bit more simple. The { silent: true } should be passed through from higher level methods so that it should be possible to just do:

Davis.location.replace(lastRequestPath, { silent: true })

or even better

req.redirect(lastRequestPath, { silent: true })

I will put together a release that fixes the bug that prevents this. I can also add this plugin to the repository so at least there is an easy to use option for people who do want this functionality. I'm happy to hear arguments for including it in the main code base too.

[Valloric]

I see this merely as a bug of the current implementation; the docs hosted on davisjs.com clearly state the following:

Before filters can be use to modify the request before the normal callback is called. They can also be used to prevent any other matching route from being called by explicitly returning false. This can be useful to ensure that some precondition is met before a route is invoked." (emphasis mine)

And this is a very useful feature (when working correctly) for many different reasons. The use case that I have is that the user is editing a form and then tries to navigate away from the page before hitting Save. I throw a dialog saying "Navigating away from this page will discard your unsaved changes. Proceed?" with a OK and Cancel buttons. If the user hits Cancel, then we want to stop the navigation. I don't see this as an uncommon use case.

Currently, if the before filter returns false, the user will stay on the page but the location in the address bar will change to the location of where he would have ended up had the navigation succeeded. This is a poor user experience; obviously, the location should not change.

I don't think this functionality should be accessible from any kind of plugin at all; I believe Davis should provide the functionality its docs talk about out-of-the-box: returning false from a before filter should stop the navigation.

[twifkak]

Thanks for the plugin, Oliver. I'm curious: what is the use case for halting the request but leaving the URL? The only thing I can think of is 403, but it seems to me that code would be better off in the controller, as it is controller-specific. (A global authz before filter that applies to all routes would probably just have a big if-else chain on the URL anyway.)

[arxpoetica]

To reiterate Valloric, "I don't see this as an uncommon use case." The use case is common.

My use case, though, is actually a little unusual (or maybe it isn't), and may actually involve another feature request/pull. I've got a real-time application built in SocketStream/Node.js. All routing is handled on the front end. When a user hits a page, if, for some silly reason, they hit the same request via navigation I don't want the page recurring the same action. I want status quo (this is the part that may need another feature request).

So by returning false it should do nothing, yes? In the current implementation, it adds the same page to the history.

I like the idea of being able to control if the history gets added or not, though. But I would think default should be no history addition.

[arxpoetica]

Also, thanks for the code. :)

[arxpoetica]

olivernn, what's the verdict on this?

[olivernn]

I've been giving this a bit of thought, and I can see that there is a need for this functionality, whether that fits into the before method or not isn't as clear. If this did go into before then it is quite a change to the behaviour of the method, and I'm still not convinced it is the right place for it.

Instead I think there could be a method called guard (or similar) which acts in the same way as a before filter except that it can automatically 'reset' the current location if the guard fails.

I have some ideas of how Davis could handle both of these kind of cases, and others #67 better, using more of a middleware approach. This is work that will go into a 1.0 release of Davis, which should be happening in the next couple of months, its just a matter of finding some time!

I think for now the plugin that I posted above should be enough to give you the desired functionality, and the use case will be addressed in the next major version of Davis.

[Valloric]

IMO, guard sounds like a great idea. It's simple and obvious.

Thanks for still considering this issue.

On Thu, Jan 17, 2013 at 9:56 AM, Oliver Nightingale < notifications@github.com> wrote:

I've been giving this a bit of thought, and I can see that there is a need for this functionality, whether that fits into the before method or not isn't as clear. If this did go into before then it is quite a change to the behaviour of the method, and I'm still not convinced it is the right place for it.

Instead I think there could be a method called guard (or similar) which acts in the same way as a before filter except that it can automatically 'reset' the current location if the guard fails.

I have some ideas of how Davis could handle both of these kind of cases, and others #67 https://github.com/olivernn/davis.js/issues/67 better, using more of a middleware approach. This is work that will go into a 1.0 release of Davis, which should be happening in the next couple of months, its just a matter of finding some time!

I think for now the plugin that I posted above should be enough to give you the desired functionality, and the use case will be addressed in the next major version of Davis.

— Reply to this email directly or view it on GitHubhttps://github.com/olivernn/davis.js/issues/64#issuecomment-12380137.

[arxpoetica]

+1

[arxpoetica]

Bump bump. What's the status on this issue?

[arxpoetica]

Still haven't heard. Is Davis.js abandonware?