MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
14.02k stars 925 forks source link

Bug with m.route redirect? #640

Closed gilbert closed 7 years ago

gilbert commented 9 years ago

It seems that it is not possible to redirect from one variadic route to another. My use case is I want to ensure a certain variadic route always ends with a trailing slash. If it does not, then redirect to the same route that does. Here is the example:

m.route(document.getElementById('app'), '/', {
  '/': {
    controller: function () {},
    view: function(){ return m('h1', 'Home') }
  },

  '/:page...': {
    controller: function () {
      var route = m.route()
      if (route[route.length-1] !== '/') {
        console.log("Redirecting to", route + '/')
        return m.route(route + '/')
      }
      this.items = [10,20,30]
      // this.items = m.request(...)
    },
    view: function (ctrl) {
      return m('.app', [
        m('h1', "Page"),
        ctrl.items.map(function(x){ return m('p', x) })
      ])
    }
  }
})

This throws the error Uncaught TypeError: Cannot read property 'map' of undefined for ctrl.items.map.

Normally, when a redirect happens in a controller, Mithril will perform the redirect and not run the current view. However, in this case Mithril is rendering the view even after the m.route(route + '/'), causing ctrl.items to be empty.

I think the expected behavior should be like other m.route calls in the controller: stop the executing of the view and run the controller on the new route (in this case, the same route, which is why it might be behaving the way it is).

pelonpelon commented 9 years ago

items never gets set on redirect. Try setting items before the if statement. http://jsbin.com/befenu/4/edit

gilbert commented 9 years ago

I do understand that is what's happening. I'm saying that the behavior of this scenario is different than what normally happens with m.route, which is to not render the view (when redirecting), and run the next controller/view instead.

pelonpelon commented 9 years ago

Ah. Sorry. I didn't understand the problem correctly.

View is called because you return from the controller.

If you want to short circuit the call to view you should just call m.route outside a return statement and return nothing from the controller. I believe that makes it consistent with what normally happens during redirects.

http://output.jsbin.com/befenu/6?/1

// the '1' at the end of that url is a page number /:page

gilbert commented 9 years ago

When the controller initializes, it returns no matter what.

How about this example:

m.route(document.getElementById('app'), '/bad', {
  '/bad': {
    controller: function() {
      m.route('/')
    },
    view: function () {
      throw Error("I never get here!")
    }
  },
  '/': {
    view: function() { return m('h1', "Hello World") }
  }
})

In the above example, no matter what, the error you see will never get thrown. This is because Mithril does not render the view when you redirect in the controller via m.route.

Normally, that is. As with the original report, it seems if you redirect from a variadic route to the same variadic route, the behavior is then different.

pelonpelon commented 9 years ago

In my example above, http://output.jsbin.com/befenu/6?/1 redirects to http://output.jsbin.com/befenu/6?/1/ View is not called on the first route /1 but it is called once on the new route /1/ -- which I think is what you want.

That seems like normal redirect behavior.

I'm obviously not understanding the problem.

gilbert commented 9 years ago

In your example, the view renders before the redirect happens. It should render after instead.

gilbert commented 9 years ago

You can test this by changing the m.route(route + '/') to return m.route(route + '/')

pelonpelon commented 9 years ago

This is the signature of m.route: void route(DOMElement element, Boolean isInitialized, Object context, Object vdom) As you can see, it returns void (undefined). When you return "undefined" from the controller, ctrl.items means nothing in the view. Your app throws an error trying to map over nothing.

gilbert commented 9 years ago

Please see my previous code example with the /bad route. In route normal behavior, the error in that example never gets thrown because the view does not run before the redirect. It only runs after the redirect.

In my application, I return early so that I do not make an extra AJAX request.

pelonpelon commented 9 years ago

I think /:page is catching more than you expect. The flow goes something like this: http://jsbin.com/befenu/13/edit?js

/ -> home -> /
/bad - >bad.controller ->m.route('/') / ->default.controller ->default.view (Home)   //skips /bad view, Error never thrown
/1 ->page.controller ->m.route('/1/') ->page.controller ->page.view (Page)   //skips view first time only, Error not thrown first time through

When you go to a variadic route (i.e. '/1), there are two redirects. The first '/1' gets caught in the controller and page.view is never called. Just like /bad and just as you expect. On the second redirect '/1/ the if statement is satisfied and page.controller passes the return value "undefined" to page.view. The map fails because ctrl.items is undefined.

In the jsbin above, notice m.route.param("page") -- Mithril matches both '/1' and '/1/' with /:page... Also notice the counter I added only gets logged on the second trip through page. The Error never gets thrown.

Hopefully this helps

gilbert commented 9 years ago

Ah ok, so it seems that the view render timing is behaving normally. Instead, what's happening is Mithril is running the controller twice, but passing the first result of those runs into the view.

I imagine this is a bug, since normally the view gets the latest result, as opposed to the first.

pelonpelon commented 9 years ago

That's not how I understand it. I believe It's passing the second (last) result of controller into the view. The first result of controller is undefined, but the view is not called so undefined disappears into the ether. From the second (last) call to the controller return m.route(route + '/') returns undefined as per the signature of m.route(): void route(DOMElement element, Boolean isInitialized, Object context, Object vdom)

As an example, in this jsbin, on the second call to controller (counter === 2) we return the route explicitly (what you are expecting to be the result of m.route) instead of undefined. http://jsbin.com/befenu/16/edit?js,console

gilbert commented 9 years ago

Ah, but it is the first version! :) Click one of the /var routes in this fiddle: http://output.jsbin.com/kijaduluya?/

The result is the first version of the controller. Here's the JS code for the record:

var menu = m(".menu", [
  m('a[href="/var/1"]', { config: m.route }, "/var/1" ),
  m('br'),
  m('a[href="/var/2"]', { config: m.route }, "/var/2" ),
  m('br'),
  m('a[href="/"]', { config: m.route }, "/home" )
])

m.route(document.getElementById('app'), '/', {
  '/': {
    controller: function () {
      console.log('/ controller');
    },
    view: function(){
      console.log('/ view');
      return m('div', menu)
    }
  },
  '/var/:page...': {
    controller: function () {
      console.log('/var/page... controller');
      console.log('page = ' + m.route.param("page"));

      var route = m.route()

      if (route[route.length-1] !== '/') {
        console.log("Redirecting to", route + '/')
        this.version = 'first'
        m.route(route + '/')
      }
      else {
        this.version = 'second'
      }
    },
    view: function (ctrl) {
      console.log("/var/page... view")
      console.log("CTRL:", ctrl)
      return m('h1', "Controller version: " + ctrl.version)
    }
  }
})
pelonpelon commented 9 years ago

A redraw happens at the start of an animation frame. The same controller can be called multiple times between frames but its view will be rendered only once. The rendered view takes the return value of the most recent call of its controller.

Here is the surprising part The flow is like this

m.mount `/var/1` controller1.version = first
    m.route `/var/1/` ->window.location
        m.mount `/var/1/` controller2.version = second
        controller2 returns (second)
controller1 returns (first)

On the next available animation frame the view is rendered with controller1 as its argument. Watch this in action with a few extra console.logs. You get better information with devtools. http://jsbin.com/neleha/10/edit?js,console,output

Here is your example exactly with the exception that I've slowed down the redirects with setTimeout. http://jsbin.com/neleha/4/edit?js,console,output You can set the timeout to 0 and still get the same result. Each controller initialization and its corresponding view land on different animation frames. The second one wins

I don't know if I'd call this a bug, an idiosyncrasy maybe.

gilbert commented 9 years ago

Wow, this is actually really intricate. So what makes this case different than the original /bad route example?

pelonpelon commented 9 years ago

The view of the /bad route ends up on in the same animation frame with /. Redraw can only call one view, so it calls the last one. (Last in, first out).

This is really not a Mithril issue, per se, although it's a built in feature. You don't have to worry about jank or poorly performing animations. It is a consequence of trying to achieve 60 fps with requestAnimationFrame (rAF).

If redraw doesn't wait for the next available animation frame it will take up the remaining part of the current animation frame, part of the next one, and the last part of the second animation frame is wasted. That is, 2 animation frames are used up for one redraw. The user perceives 30fps per second instead of 60fps.

Animation frames: |____|____|____|____|____|____|
Poorly timed      |   ----      ----      ----  |
using rAF         |---- ---- ---- ---- ---- ----|

This article can explain it much better than I can: http://www.nczonline.net/blog/2011/05/03/better-javascript-animations-with-requestanimationframe/

gilbert commented 9 years ago

I don't understand though. In both cases exactly one redirect is happening. How does the variadic route case cause an extra animation frame?

pelonpelon commented 9 years ago

The variadic route doesn't cause an extra animation frame. That's the point. I think my explanation of requestAnimationFrame was a distraction. I only wanted to point out why redraws occur periodically instead of immediately when expected.

Mithril's default is to not cause the extra animation frame, thus on any given component, if the view is called multiple times before the next animation frame, all are discarded except the final one. It's like a queue with length = 1.

If you want every call to view (on the same component) to be rendered, you must spread those calls out on separate animation frames -- redraws (which Mithril schedules when an animation frame is ready).

https://github.com/lhorie/mithril.js/blob/next/mithril.js#L622

In the case of your original question, the real problem is that the calls to m.route do not complete in the order expected, as I've outline above. "first" completes after "second", and since both views are called, in that order, before the next animation frame is ready, "first" has bumped second out of the queue and is rendered.

gilbert commented 9 years ago

@pelonpelon Do you mean "controller" when saying "called multiple times" ? The view is already being called only once. It's the controller that is being called twice (as expected), but the second controller is being discarded in favor of the first.

Here's a non-anonymous created version of my previous jsbin: http://output.jsbin.com/gosuduk/1?/

pelonpelon commented 9 years ago

I mean views. Maybe calling and discarding is not the right phrasing (short-circuited ?). Here's my last ditch effort to outline what I believe is happening:

m.mount /var/1               // controller1.version = first
    m.route /var/1/          // ->window.location
        m.mount /var/1/      // controller2.version = second
        controller2 returns  // (version=second)
          view2 becomes the render candidate for the next redraw()
controller1 returns          // (version=first)
  view1 replaces view2 as the render candidate
redraw() is finally called, view1 is rendered with version=first

Alternatively, with setTimeout, redraw() is called twice, once on 2 separate animation frames (A,B):

I'm obviously not explaining this well. I think I understand it correctly, but maybe I'm missing something. Perhaps someone else can chime in and do a better job.

nijikokun commented 8 years ago

I am currently encountering this issue. Should you redirect early in the controller, view is still invoked when it should not.

A state machine could solve the issue, to state that we are redirecting and to omit the previous request to render a view.

I wonder if this applies to Mithril next

nijikokun commented 8 years ago

To update I do something along the fashion of:

  if (shouldRedirect) {
    m.endComputation()
    m.route('/blah')
    return { redirected: true }
  }

Then check the redirected value in the view to ignore this pass.

barneycarroll commented 8 years ago

@nijikokun you could probably do one better by simply nooping the current draw with m.redraw.strategy( 'none' ) in that condition.

dead-claudia commented 7 years ago

Closing - last v0.2 release has been cut.