Skip to content
This repository was archived by the owner on May 25, 2019. It is now read-only.

[RFR] Fix Strict DI issues and remove ngAnnotate#127

Open
jpetitcolas wants to merge 1 commit into
angular-ui:masterfrom
jpetitcolas:di
Open

[RFR] Fix Strict DI issues and remove ngAnnotate#127
jpetitcolas wants to merge 1 commit into
angular-ui:masterfrom
jpetitcolas:di

Conversation

@jpetitcolas

@jpetitcolas jpetitcolas commented Sep 13, 2016

Copy link
Copy Markdown

This PR aims to solve an issue when importing this module in a Strict Dependency Injection environment. Indeed, using current version, installed with npm, we get the following Angular error:

Error: [$injector:strictdi] http://errors.angularjs.org/1.4.12/$injector/strictdi?p0=uiCodemirrorDirective

Bower version is not impacted as ng-annotate plugin seems to be used in this case.

As source file is already compatible with strict DI, I also removed the obsolete ng-annotate plugin.

@jpetitcolas

Copy link
Copy Markdown
Author

Fixes #106.

@genu

genu commented May 9, 2017

Copy link
Copy Markdown

Not sure why removing ng-annotate is the solution here. I figure it should work with ng-annotate as expected in npm as long as the main file points to the dist version. #138 Attempts to make the same change.

Also, why aren't these PRs reviewed? Looks like this hasn't been touched for a year.

@jpetitcolas

Copy link
Copy Markdown
Author

@genu: the main fix is here.

Removing ng-annotate has been done because it is not required anymore, as code is compatible with strict DI. But that's a side-effect of the fix.

@postama

postama commented Mar 19, 2018

Copy link
Copy Markdown

What is the status of this PR. It has been almost a year and this fix is necessary for us. Let me know if there is anything that needs to be done to allow it to get merged in.

@jpetitcolas

Copy link
Copy Markdown
Author

Same here. We got a production project using my fork as a dependency. That's not really comfortable. :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants