search

angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.85k stars 27.52k forks source link

Angular 1.3 does not respect controller instance returned #13069

Closed wesleycho closed 8 years ago

wesleycho commented 8 years ago

With UI Bootstrap, we introduced deprecation messages - here is one example of such.

However, we support Angular 1.3 & 1.4 currently. 1.4 currently correctly respects the returned instance as the controller itself, whereas 1.3 does not - the commit that introduces this fix in 1.4 is 62d514b06937cc7dd86e973ea11165c88343b42d.

Is there any way this fix can be backported and released in 1.3? Otherwise we will be forced to duplicate the controller block for all similar injectables, which would be painful for users in a drastically bloated library as a result to fix this problem. It should be noted that there are also issues with simply extending the existing controller with the new one instantiated by $controller. I attempted to cherry-pick the fix with adaptations, but it appeared to introduce a memory leak - there is likely something else going on there with changes to the compiler introduced in 1.4.

Narretz commented 8 years ago

We can certainly give it a shot, but since we are at 1.5.x already, it's definitely not guaranteed. Can you please open a PR with your cherry-pick as a starting point?

That said, wouldn't it be a good time to stop supporting 1.3.x in ui bootstrap? The changes between 1.3 and 1.4 are far less pronounced than before (no browser support is dropped), so I'd rather have people upgrade to 1.4 than support it with backports.

pkozlowski-opensource commented 8 years ago

That said, wouldn't it be a good time to stop supporting 1.3.x in ui bootstrap? The changes between 1.3 and 1.4 are far less pronounced than before (no browser support is dropped), so I'd rather have people upgrade to 1.4 than support it with backports.

We are "officially" supporting on 1.4 but people keep using 1.3.x.... I guess some still didn't take time to upgrade...

Foxandxss commented 8 years ago

The problem I see in here is: You make a massive work (trust me, was a hell of a ride) to prefix all your directives but also maintaining compatibility with your current users. There is lot of fiddling in there. It is not only having two copies of each directive / service / controller. There are some components that are generated dynamically (so you need to make sure that the deprecated one is till generated successfully), there are directives that are generated with CSS classes (also to make sure that deprecated directives are still working). Also make sure that you don't have css classes that could match deprecated directives (#13057). In short, it is rather complicated to have two set of directives coexisting in the same place and not having them fight.

After dozen of hours, we manage to achieve that to then realize that one simple piece of code is not compatible with 1.3.x.

Since we are on 1.5.x already I understand the angular team position, but after fighting that hard to not break compatibility with current users, I think that we (ui-bs team) should restore 1.3.x compatibility, even if that adds a bit of more temporary bloat. On the best case scenario, we will remove all deprecated code before the year ends.

Anyhow, I think that simply swapping return $controller(...) for angular.extend(this, $controller....) works in all cases but one. I think that the overhead is going to be minimum.

wesleycho commented 8 years ago

I'm going to close this request, as we were able to adequately work around this (a little more bloat unfortunately), and it probably was a singular use case.