[WIP] Refresh href in linkTo when one of the params or properties change

[drogus]

I've already opened a similar pull request here #2181, but as I mentioned in the description there, it didn't handle the case when an attribute changes. This pull request is not technically ready, I would need to add more tests and I would need to make changes in vendor/router.js in the actual router repo, but I submit it for discussion (specifically, if this has chances to be merged in).

The problem, which this PR tries to resolve is described here: #2241. To fix it, we need to refresh the href attribute whenever one of the properties used to generate it changes.

The idea behind this is that in most cases, when dynamic segments are used, serialize method needs an id to generate an URL. In the rare cases, when it needs something else, we could require a bit more declarative approach:

App.PostRoute = Ember.Route.extend({
  serializePaths: ['slug'], // I suck at naming, halp!
  serialize: function(post) {
    return { slug: post.get('slug'); }
  }
});

One more addition, which I would do on top of things already available in this PR is to set # as an URL if any of the params or properties is null. This would allow to just forget about problems in #2241.

Imagine the case where you display a list of posts (including links) and you insert a newly created post onto the list. Now you need to generate an URL for the new post, but id is obviously not available yet. You can do what I suggested in #2241 and just cover the link with {{#if post.id}}, but now you don't have the link at all. The ideal thing to do would be to render the link with # as a href and then update it as soon as record is saved and has an id.

[ilovett]

Woohoo! +1

[drogus]

/cc @wycats @tomdale @wagenet

I've been thinking about this PR a bit and I think that currently this is the best way to handle the problem.

The facts are:

1. People encounter the problem of non-refreshing linkTo helper
2. The solution is not "straightforward", ie. there is no framework support for solving the problem, the only thing you can do is to wrap your links with ifs and withs

The solutions that can think of are:

1. Do nothing
2. Use the solution in this PR
3. Try to come up with the other solution (like: make serialize computed proeprty).

1. is quite bad, because people have this problem and more people will encounter it, then they will either ask for help or file an issue, because it indeed looks like a bug at the first glance.

@wycats had doubts about 2), because of the necessity to use serializePaths, like here. Indeed, this is a small problem, because people using something else than id, need to know that they need to declare such fact. I believe that a good docs for serialize and mentioning it in guides and official tutorial will be enough to make the effect minimal.

The third solution would be best if we could do it without changing the current API. If we need to change the API, we're back at the problem from 2) and I don't see any way of solving this problem without changing the API.

To sum up, I believe that merging this patch will fix the situation for majority of cases, because id is usually used for the URLs. For all the other cases, where non-id fields are used to generate the URL, I think that some of the people will find answer in the docs and even if not, it's much better to point them to serializePaths than say: "this is not a bug, you need to solve it by yourself" (which we currently need to do).

In order to merge this, I will need to implement pathsForSerialize in the router.js, currently I changed vendor/router.js. If I have green light on this, I will implement this properly in the router itself (or this could be a more general case method like router.eachHandler).

[ilovett]

Disclaimer: I am just getting started with Ember. Please correct me if I am wrong...

It looks like your code takes that list and creates observers for each one provided in serializePaths...

Could we not automically create observers for all paths returned in serialize object other than id? Would this be a performance issue? Can you think of any use cases where this wouldn't be wanted?

[drogus]

Could we not automically create observers for all paths returned in serialize object other than id?

If you don't create an observer for ids, it will not work when you change context (for example you display a page for Post with id equal 1 and you swap the Post with another one)

Would this be a performance issue? Can you think of any use cases where this wouldn't be wanted?

I doubt that this will be a performance issue, it will usually be 1 or 2 additional observers per link and if you want it to work correctly you often have to use {{if}} or {{with}}, which is more costly - now instead of just observing a property and refreshing href ember needs to manipuate the DOM.

[ilovett]

So is it possible to have the serializePaths array built implicitly from the keys of route.serialize rather than having to specify them? Since reason 2 doubt was due to having to add it.

[drogus]

@ilovett nope, I don't see any way to build serializePaths automatically, but you have to specify it only for non ids routes.

[ilovett]

I am interested in contributing to Ember but have not yet played around with the source... so again, please educate me :)

Couldn't you modify the Ember.Route constructor / init to send a dummy model to the serialize function, which would return an empty dict of the keys you want, then build the array from that?

[ilovett]

Thinking about it more, I suppose the serialize function could do other things with the model before returning... which might throw an error... This suggestion is little hacky but wouldn't require API change.

[drogus]

@ilovett I was thinking about such recording. One of the issues is what you mentioned, serialize could call an arbitrary function on the model, which would fail if this is just a simple recording object. It could also do something like:

serialize: function(model) {
  var somethingElse;
  if(somethingElse = model.get('somethingElse')) {
    return { something_else: somethingElse.get('id') };
  } else {
    return { id: model.get('id') };
  }
}

With serializePaths it could be solved with serializePaths: ['id', 'somethingElse.id'], with recording it would be harder to achieve - maybe not that hard, but I'm pretty sure you can come up with a lot of examples which would be hard to handle.

[machty]

This seems a little too special purpose for router.js; I think you could leave this in the system/router.js file and run pretty much the same code. Use this code for reference (even though it's being removed since it's not being used).

[drogus]

Good point, I'll do that.