Ember.js: Router `transition.retry` with `refreshModel` queryParams causes unexpected transition sequence

Created on 23 Apr 2015 · 19Comments · Source: emberjs/ember.js

Please bare with me. This is kind of a hard issue to explain, but I do have a jsbin to help demonstrate it.

We discovered an issue when a user refreshes a page that has queryParams configured with refreshModel and there is a transition.retry involved. The scenario is someone deep-linking into the app with queryParams, but first being redirected to the login page.

It turns out that after logging in and calling retry, all of the resources/routes with dynamic segments have their serialize hooks called with undefined model values instead of calling the model hook with the params. Here is the internal transition log for the router, it shows entirely different transition paths: https://gist.github.com/workmanw/20a5a2a667f2bb9d27a7 .

The best way to demonstrate this issue is clicking the jsbin link below. You'll see it first redirects you to a login page, then a timer "simulates" login after 1 second, then it triggers the transition.retry that causes the serialize hook to be called with an undefined model. Then if you repeat the process without a queryParam, it works just fine.

With queryParam: http://emberjs.jsbin.com/gonadewaki#/post/1/comments?search=awesome
Without queryParam: http://emberjs.jsbin.com/gonadewaki#/post/1/comments

In my debugging, I discovered that if set refreshModel back to false or if didn't redirect to login, it works as expected. So this bug is a bit obscure.

Hopefully that is a clear explanation.

EDIT Here is a link to the same issue on canary for quick update testing: http://output.jsbin.com/humibo/1#/post/1/comments?search=awesome

Bug Query Params

Source

workmanw

Most helpful comment

:clap: :clap: :clap: :clap:

workmanw on 31 Mar 2016

🎉2

All 19 comments

We are experiencing the exact same problem with our project. It uses ember-simple-auth for it's login redirection logic (which under the hood uses transition.retry()) If we set refreshModel to false the problem goes away (obviously the queryParams no longer work right).

We have found that with the above scenario transition.retry() some how effectively performs as if transitionTo('previousTransitionRoute', undefined) causing it to bypass the route's model hook and instead resolve the model to undefined which is obviously very wrong.

Is it possible that the retry causes an _undefined_ model because queryParams unintentionally creates a TransactionAborted for each query param set to refreshModel:true? (See issue #5566)

sukima on 5 May 2015

Any updates on this?

seawatts on 26 May 2015

@seawatts Nope. I spent some initial time trying to resolve it, but I've been strapped with work the past few months.

workmanw on 26 May 2015

@seawatts Our solution was to drop any use of refreshModel and instead setup an observer on the indevidual query params and do the refresh ourselves (wrapped in an Ember.run.once).

EDIT: We also found a situation where the route's model hook would get triggered on multipl Ember.run.once so we had to implement a manual debounce flag using setupController.

Route

import Ember from 'ember';

export default Ember.Route.extend({
  model: function() {
    // ...
  },

  setupController: function(controller, model) {
    // ...
    controller.set('queryParamsReady', true);
  },

  actions: {
    issueQueryChanged: function() {
      this.refresh();
    }
  }
});

Controller

import Ember from 'ember';

const QUERY_PARAMS = ['foo', 'bar', 'baz'];

export default Ember.Controller.extend({

  needs: ['application'],

  queryParams: QUERY_PARAMS,

  queryParamsReady: false,

  issueQueryChanged: Ember.observer(...QUERY_PARAMS, function() {

    // CurrentPath is null of the application is loading for the first time.
    // Forcing a reload during the loading state would be needless since we're
    // already going to load the model.
    var hasCurrentPath = this.controllerFor('application').get('currentPath');
    if (hasCurrentPath && this.get('queryParamsReady')) {

      // Prevent multiple query param changes from triggering reload multiple
      // times
      this.set('queryParamsReady', false);
      Ember.run.once(this, this.send, 'issueQueryChanged');

    }
  })
});

Hope it helps.

sukima on 27 May 2015

Thanks @sukima! I've added some edits to your suggestion. There was also a deprecation that I fixed.
My current implementation doesn't require you to predefine QUERY_PARAMS and you can use it with Ember's current implementation of queryParams in both route and controller. You only need to change refreshModel: true to refresh: true.

Route

import Ember from 'ember';

export default Ember.Mixin.create({
  setupController: function(controller, model) {
    this._super.apply(this, arguments);
    controller.set('queryParamsReady', true);
  },

  actions: {
    issueQueryChanged: function(changedParam) {
      if( this.queryParams && this.queryParams[changedParam] && this.queryParams[changedParam].refresh ) {
        this.refresh();
      }
    }
  },
});

Controller

import Ember from 'ember';

export default Ember.Mixin.create({
  needs: ['application'],

  queryParams: [],

  queryParamsReady: false,

  init: function() {
    var that = this;
    this.get('queryParams').forEach(function(param) {
      that.addObserver(param, that.issueQueryChanged);
    });

    this._super.apply(this, arguments);
  },

  willDestroy: function() {
    var that = this;
    this.get('queryParams').forEach(function(param) {
      that.removeObserver(param, that.issueQueryChanged);
    });

    this._super.apply(this, arguments);
  },

  issueQueryChanged: function(sender, key/*, value, rev*/) {
    // CurrentPath is null of the application is loading for the first time.
    // Forcing a reload during the loading state would be needless since we're
    // already going to load the model.
    var hasCurrentPath = this.get('controllers.application.currentPath');
    if (hasCurrentPath && this.get('queryParamsReady')) {

      // Prevent multiple query param changes from triggering reload multiple
      // times
      this.set('queryParamsReady', false);
      Ember.run.once(this, this.send, 'issueQueryChanged', key);
    }
  },
});

Usage

E.g. a search query parameter.

Route

import Ember from 'ember';
import Route from './route';

export default Ember.Mixin.create(Route, {
  queryParams: {
    query: {
      refresh: true
    },
  },
  // ...
});

Controller

import Ember from 'ember';
import Controller from './controller';

export default Ember.Mixin.create(Controller, {
  init: function() {
    this.queryParams.push('query');
    this._super.apply(this, arguments);
  },

  query: '',
  searchQuery: '',

  isSearch: function() {
    this.set('searchQuery', this.get('query'));
    return this.get('query').length > 0;
  }.property('query'),

  actions: {
    search: function() {
      this.transitionToRoute({
        queryParams: {
          query: this.get('searchQuery').trim()
        },
      });
    },
  },
});

louy on 4 Jun 2015

Shameless Bump. I know everyone's been busy with all the glimmer and 2.0 stuff, but this is still an issue for us. I spent 30 minutes trying to figure this out, but all of the async and promise architecture leads me to believe it requires a good amount of ramp up to figure this out.

I know a lot of the code is likely to be refactored with routable components, but it looks like that's at least 3 releases out. I'd really like to see if there is an easy fix. Any advice would be really appreciated.

workmanw on 14 Jul 2015

I've run into this issue as well. It's especially frustrating since it breaks an example provided in the guides: http://guides.emberjs.com/v1.13.0/routing/preventing-and-retrying-transitions/#toc_storing-and-retrying-a-transition

Bargs on 5 Aug 2015

Shameless ping @machty

A new feature in our app has caused this bug to surface in a major way. I'd love to help get it fixed and I'm willing to dive in, just need a little direction.

workmanw on 21 Dec 2015

Would be nice to have it fixed, I was pulling my hair out as I did not understand how I could have errors because of undefined model reported to getsentry.

Thanks @sukima and @louy for your workaround