Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Angular 1.3 does not respect controller instance returned #13069

Closed
wesleycho opened this issue Oct 11, 2015 · 4 comments
Closed

Angular 1.3 does not respect controller instance returned #13069

wesleycho opened this issue Oct 11, 2015 · 4 comments

Comments

@wesleycho
Copy link
Contributor

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 62d514b.

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
Copy link
Contributor

Narretz commented Oct 11, 2015

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants