dotJEM / angular-routing

Enhanced state based routing for Angular applications!
https://dotjem.github.io/angular-routing/
MIT License
75 stars 9 forks source link

Support for AngularJS 1.3 #112

Closed Cinamonas closed 9 years ago

Cinamonas commented 10 years ago

Hi,

I know that the library does not officially support 1.3 (at least according to README, which says that AngularJS 1.2 is required), but it was working well up until the latest release – v1.3.0-beta.18. So maybe it is worth investigating if it worked with all other 1.3 releases.

Cinamonas commented 10 years ago

I decided to investigate this issue and I found that $view.get makes a copy of view before returning it. AngularJS had some major refactoring of promises in v1.3.0-beta.18 and now when you copy a promise and resolve deferred afterwards, copy stays unresolved. Previously, both original promise and copy would be resolved. Thus with current implementation, view.template.then callback is never executed.

I'm not sure if this new behaviour is correct, so I have opened an issue for AngularJS: angular/angular.js#8726.

jeme commented 10 years ago

Yes, I have decided to hold back on 1.3 until they are out of beta, so no guaranties. Bur people are always welcome to try it and report findings.

How copying promises should work is an interesting discussion, but lets wait and see what their response to that is.

What we can do is to patch in their old copy (that that is what has changed) or implement copy our self.

vicentereig commented 10 years ago

This is just a side-note to the current $view.get implementation.

Also avoiding making a copy of that view would allow people to use Ember models within an Angular app and angular-routing. I did some experiments a few weeks ago a documented it on this reddit thread.

It's a pretty cheap solution, but I'm currently bypassing this by decorating the $view exposing a $view.get that actually clones the view but doesn't clone the locals section.

view.locals gets duplicated, removing in consequence the Ember Object wrapper around the model, stripping out the ability to compute properties.

Regardless the Angular 1.3 compatibility, it would be awesome to see this corrected.

@jeme Just out of curiosity, why did you decided to clone the $view at that point? I've been diving into the router implementation, but I don't feel 100% sure to provide the correct implementation.

jeme commented 10 years ago

@vicentereig the rationale behind cloning before returning has to do with keeping the internal state intact regardless of what you do with the object out of the get, allowing people to do with it what they wan't, since many of the components has been developed with the mindset that they aren't just something the module uses internally, but something developers can also use.

We can take different approaches to this, one could be to allow you to disable this behavior completely. Or the reverse, making it a thing you can enable, but that is disabled by default, since it doesn't necessarily make as much sense for views as it does for states. (Which is where it inherited that behavior from)

Cinamonas commented 10 years ago

Don't mind these references to my fork – it's a, hopefully, temporary patch for my needs :wink:

thorn0 commented 10 years ago

If it's the only thing that is incompatible with 1.3, I'd propose to fix it and not to wait until 1.3 is released.

jeme commented 10 years ago

@thorn0 I would suggest that efforts goes towards the Core framework instead, see if they would agree that angular.copy should respect promises, or if they think another solution would be preferred.

As that would fix this issue as well as being beneficial to all who uses angular, not just something internally to this framework.

If they don't think there is any issue, then obviously we will need handle this internally, so right not this waits for clarification.

vicentereig commented 9 years ago

@jeme :+1: Thanks man, keep on rocking it!