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

Using Promises as Route models #1642

Closed
ghost opened this issue Jan 1, 2013 · 32 comments
Closed

Using Promises as Route models #1642

ghost opened this issue Jan 1, 2013 · 32 comments
Milestone

Comments

@ghost
Copy link

ghost commented Jan 1, 2013

Returning a Promise from Ember.Route.model() seems to work correctly only when the Router is first initialized. I.e. when the application is loaded with a deep link to the route and a given model. (The previous Router switched to the 'loading' state when deserialize() calls resulted in a Promise, IIRC, but I haven't dug deep enough into the new API yet to fully understand how this works now.)

Anyway, subsequent calls to Ember.Route hooks such as setupControllers() result in the Promise being delivered as the model.

One can get around this fairly easily by doing something like this:

setupControllers: function(controller, model) {
  if (model && Em.typeOf(model.then) === 'function') {
    model.then(function(model) {
      controller.set('content', model);
    });
  } else {
    controller.set('content', model);
  }
}

But it seems to me that the Ember.Route hooks should be Promise-aware by default.

@wycats
Copy link
Member

wycats commented Jan 1, 2013

Currently, promises are used only when you enter the router with a URL. In this state, an unresolved promise is relatively easy to handle: put the entire router in a loading state (App.LoadingRoute).

If you are transitioning from one route to another and get a promise, you probably want to handle loading on the target state. You wouldn't want to change your UI into a loading spinner just because your content wasn't ready to render yet.

The current strategy is to allow loading-related hooks on route handlers that return promises for models (probably something like modelIsLoading, modelDidLoad, and modelWasRejected. You could then use the render API in those hooks.

Do you have any thoughts on that strategy?

@lukemelia
Copy link
Member

@wycats, am I understanding correctly that modeIsLoading would be a substate of the route, and modelDidLoad and modelWasRejected would be events on that state?

If so, I think that is a good approach, esp with default implementation of the state so that it works out of the box with no code but can be enhanced by implementing the state.

@ghost
Copy link
Author

ghost commented Jan 2, 2013

My initial confusion stemmed from the fact that promises were handled differently when entering the router via a URL versus a transition between routes.

I would agree to have promise-aware hooks in Ember.Route, but creating a substate would perhaps be overkill.

I've attempted to illustrate the idea of loading/promise-aware hooks in code, based on my own understanding of the new routing so far.

App.PromiseAwareRoute = Ember.Route.extend({

  deserialize: function(params) {
    model = this.model(params);
    if (model && Em.typeOf(model.then) === 'function') {
      // Clear the currentModel explicitly, so that modelFor() does not return the promise.
      this.currentModel = null; 
      // Retain the promise for possible internal use.
      this.promiseModel = model;
      this.handlePromiseModel(model);
    } else {
      this.currentModel = model;
    }
    return model;
  },

  model: function(params) {
    promise = App.EntityFinder.findById(params.id);
    return promise;
  },

  handlePromiseModel: function(promise) {
    var self = this;
    promise.then(function(result) {
      self.currentModel = model;
      self.modelWasResolved(result);
      return result;
    }, function(result) {
      self.modelWasRejected(result)
      return result;
    });
  },

  modelWasResolved: function(result) {
  },

  modelWasRejected: function(result) {
  }

});

The key point I want to make is that I feel it would be clearer if currentModel always referred to an actual model: not to a promise in the case of loading from a URL and, on the other hand, a model when transitioning between states.

@wycats
Copy link
Member

wycats commented Jan 2, 2013

@lfridael it's definitely wrong for currentModel to be the promise while it's loading. Good catch.

Question: why were you doing modelFor('route') for a route that was currently loading? That's the only case you should be able to observe this issue.

@wycats
Copy link
Member

wycats commented Jan 2, 2013

@lukemelia I agree with @lfridael that I'd want to start with hooks first and move to substates only if the use-cases justify it.

@ghost
Copy link
Author

ghost commented Jan 2, 2013

@wycats My remark about the use of modelFor() does not contribute anything to this discussion. :-) It's early morning over here and I haven't had coffee yet.

@ghost
Copy link
Author

ghost commented Jan 2, 2013

@wycats The promise-related hooks definitely have added value, but shouldn't the calls to Ember.Route.setupControllers() and Ember.Route.renderTemplates() be normalized as well? I.e. passed the model instead of the promise in all cases?

@ghost
Copy link
Author

ghost commented Jan 2, 2013

@wycats My solution sketch normalizes calls to the setupControllers and renderTemplates hooks. Please take a look.

@wycats
Copy link
Member

wycats commented Jan 2, 2013

@lfridael the other hooks should already only get the models.

Here's how it's supposed to work (https://github.com/tildeio/router.js/blob/master/lib/router.js#L281-L310)

  1. The router recognizes a new URL, and moves into the loading state
  2. The router starts calling the model hook for each handler
  3. If a model hook returns a promise, the router moves into the loading handler, and waits for the promise to resolve
  4. Once the promise resolves, the router continues calling model for the remaining handlers (repeat steps 2 and 3)
  5. Once all of the promises are resolved, the router calls the setupControllers and renderTemplates hooks. By now, the model is, by definition, resolved.

After writing all of that out, I think you're saying that because that doesn't happen on normal transitionTo, the hooks can sometimes get called with a promise (the entire point of this thread!)?

It seems like this issue should be addressed in the router.js code for transitionTo (https://github.com/tildeio/router.js/blob/master/lib/router.js#L81-L94), specifically to have something like collectObjects even in transitionTo.

@ghost
Copy link
Author

ghost commented Jan 2, 2013

@wycats Yes, that's the problem. The hooks get called with the promise on non-URL transitionTo calls.

I agree the issue should be addressed higher up the chain in router.js.

Also: the handling of deferred models should take into account the possibility of asynchronous operations taking an arbitrarily long time and the user navigating to another route in the meantime. The 'active' promise should then be rejected with some sort of indication that it is being interrupted by user-initiated navigation.

@wildchild
Copy link

Once all of the promises are resolved, the router calls the setupControllers and renderTemplates hooks.

@wycats, why then the following code always return a promise for me?

// ChildRoute
setupController: function() {
  this.modelFor('parent') // it's promise.
}

and when I trying to resolve it:

// ChildRoute
setupController: function() {
  this.modelFor('parent').then(function(value) {
    throw "Resolved";
  });
}

error will never be raised. Weird.

My promise:

  App.Model.reopenClass({
    find: function(model_id) {
      var promise = Ember.Deferred.create();

      $.getJSON('/models/' + model_id).then(function(data) {
        var model = App.Model.create(data);
        promise.resolve(model);
      });

      return promise;
    }
  });

Am I missing something?

@wagenet
Copy link
Member

wagenet commented Jan 15, 2013

@wildchild Ember Data models are promises so if you're using Ember Data, you'll always get promises. The reason why you don't get an error raised is because, according to the Promises/A spec, errors are caught and passed to the failure callback. You have no failure callback, so no error is raised. I'm not a huge fan of this, but we are attempting to adhere to spec. I don't see any signs that things are behaving incorrectly here.

@wagenet
Copy link
Member

wagenet commented Jan 17, 2013

It seems like @lfridael's issue is still unresolved. Reopening.

@eviltrout
Copy link
Contributor

My ticket #1856 is definitely a duplicate of this. I have to admit I found it very confusing that the behaviour with promises changed depending on whether I was doing the initial load or navigating to a new route.

Is the solution for now to have my finders always return objects with isLoaded type attributes?

@wagenet
Copy link
Member

wagenet commented Jan 24, 2013

I closed #1856 as a duplicate but it still has some useful information for when we attempt to solve this.

@southpolesteve
Copy link
Contributor

@eviltrout can you elaborate on using isLoaded attributes? Do you have a workaround for this? I am also using my own finders. I have been struggling a lot with ember-data and the API I am using

@sly7-7
Copy link
Contributor

sly7-7 commented Jan 24, 2013

I unfortunately missed this one :/. I've tried to debug based on #1856 jsbin. I don't know if it's usefull, perhaps all of you guys are already aware of that but...

In router.js, when transitionTo is called, it called doTransition, with updateUrl as method.

function doTransition(router, name, method, args) {
  var output = router._paramsForHandler(name, args, true);
  var params = output.params, toSetup = output.toSetup;

  var url = router.recognizer.generate(name, params);
  method.call(router, url);

  setupContexts(router, toSetup);
}

I think this method aims at eh, updating the Url, but seeing that in Ember.Router:

function setupRouter(emberRouter, router, location) {
  var lastURL;

  router.getHandler = getHandlerFunction(emberRouter);

  var doUpdateURL = function() {
    location.setURL(lastURL);
  };

  router.updateURL = function(path) {
    lastURL = path;
    Ember.run.once(doUpdateURL);
  };

and in the init of Ember.Router

location.onUpdateURL(function(url) {
    router.handleURL(url); // call to router.js handleUrl.
});

router.handleUrl is something that causes the collectsObjects method who I think is responsible for resolving promises.

The fact is the updateUrl is called in an Ember.run.once(), so I guess doTransition() goes in setupContexts before the promises are resolved. I'm sure there is a good reason to put updateUrl in a run.once(), but I can't figure why.

@eviltrout
Copy link
Contributor

@southpolesteve essentially, instead of returning a promise from your model call, you can return an object with an isLoaded attribute, initially set to false. When the load finishes, it will set the attribute to true.

Then in your template, you surround everything with {{#if content.isLoaded}} ... {{/if}}

For arrays it's a little more complicated: you have to return an initially empty array, then fill it will the elements after the load completes. It should re-render your template.

@wildchild
Copy link

It's a show-stopper in my case. Is it going to be fixed in near future?

@wagenet
Copy link
Member

wagenet commented Feb 2, 2013

@wildchild, this is marked for the 1.0 RC milestone.

@pdokas
Copy link

pdokas commented Feb 10, 2013

Bumped to 1.0 Final it seems.

@drogus
Copy link
Contributor

drogus commented Mar 8, 2013

@wagenet @wycats @tomdale what's the plan on fixing this? I would like to refactor 2 places where I currently use ProxyObject to make the code simpler and I would love to contribute a fix, but I'm not sure if there is any plan on how should it behave.

@wagenet
Copy link
Member

wagenet commented Mar 20, 2013

@drogus I don't know what the plan is. You're best of bugging @wycats and @tomdale if you can.

@wildchild
Copy link

Any status update?

@scardine
Copy link

+1: show the first record for some dataset in index by default is a pretty common pattern. Because of this bug you can't use this.modelFor('objs').get('firstObject') in a Router redirect.

@utnas
Copy link

utnas commented May 24, 2013

@scardine @sly7-7
I have the same problem for my application. I need to show the first record in index template.
I'm embarrassed because I don't have a easy way to implement this, with ember.
If you have solution don't hesitate

@scardine
Copy link

@utnas : unfortunately the best solution I found is porting the App to Angular.js. Spent a couple days to pick it up, but boy, I'm not looking back. The downs?

  • Angular docs are more pedantic, academical and a bit lacking.
  • They don't have the cute mascot

Pros:

  • No namespace clashes (have you ever made a model with an attribute called content in Ember?)
  • Intuitive: almost everything just works the way you think it should
  • Great resources: http://angular-ui.github.io/bootstrap/ is awesome

Ember has great ideas, but they could borrow some lessons about POLA from the Angular guys.

@eviltrout
Copy link
Contributor

@utnas One solution is to return an object from your model instead of an array. That object can contain the array, something like a ResultSet pattern.

This is slated to be fixed -- I was just bothering @wycats about it last weekend. However it has been open a long time and I really hope someone gets to it soon. It is sadly outside my realm of knowledge of ember internals.

@pdf
Copy link

pdf commented May 24, 2013

@utnas : unfortunately the best solution I found is porting the App to Angular.js

Sadly, that was my solution too. I dig a lot about Ember, and I may come and take another look at it for my next project, but I was spending more time in the Ember code than my own right now...

@machty
Copy link
Contributor

machty commented May 24, 2013

Hi, anyone bitten by this issue should take a look at tildeio/router.js#19 . It's almost ready and has the general blessing of Ember Core, so please comment on / critique while you can.

@drogus
Copy link
Contributor

drogus commented May 25, 2013

@machty ❤️ ❤️ ❤️

@trek
Copy link
Member

trek commented Jun 17, 2013

I believe @machty's Router facelift specifically addresses this. Please reopen if I'm mistaken.

@trek trek closed this as completed Jun 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests