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

Should $injector#annotate consider the injector's own strictDi option? #12446

Closed
marcioj opened this issue Jul 27, 2015 · 8 comments
Closed

Should $injector#annotate consider the injector's own strictDi option? #12446

marcioj opened this issue Jul 27, 2015 · 8 comments

Comments

@marcioj
Copy link

marcioj commented Jul 27, 2015

For instance:

injector = angular.injector(['ng'], true); // true means use strict-di
function foo($rootScope) {}
injector.invoke(foo); // correctly throws Error: [$injector:strictdi] 
injector.instantiate(foo); // correctly throws Error: [$injector:strictdi] 
injector.annotate(foo); // ["$rootScope"]

In the last example, I also expected an Error: [$injector:strictdi] but this option is just ignored and the function is annotated.
Using injector.annotate(foo, true); make the error threw, but in this case we are duplicating the option, and also there is situations where annotate is being used by some third party library and we cannot control this.

@lgalfaso
Copy link
Contributor

Hi, as you already wrote, injector.annotate takes two arguments, the second being if this should be a strictDI or not.

I understand that it would be more consistent to make annotate not take this second parameter and use the value used when constructing the injector, but there is a reason for this discrepancy. When writing tests, using the automatic injector mechanism is cleaner. This is it should be possible to write tests that use the automatic DI even when initializing the unit test to be strictDI=true. This is a very common scenario as it serves as an additional mechanism to check that everything is properly annotated in the code being tested while at the same time not forcing some of the boilerplate needed when writing tests. This implies that when writing plugins (like karma-jasmine) that add an inject function to allow for this to happen, there needs to be a mechanism to annotate a function and that this mechanism does not follow whatever strictDI was defined to create the app injector. This is where injector.annotate comes into play.

There might be other solutions that also allow for this scenario, but I find the current trade-off reasonable.

@marcioj
Copy link
Author

marcioj commented Jul 27, 2015

Thanks for reply.
I was thinking in a solution for this, let me know if the following make sense.
What about to keep angular.injector.$$annotate in the same way, like a kind of private api to bypass the strict di for the situations you explained above. And change the injector#annotate code here to something like this:

return {
      invoke: invoke,
      instantiate: instantiate,
      get: getService,
      annotate: function (fn) {
        // this strictDi variable comes from createInjector(modulesToLoad, strictDi) method
        return createInjector.$$annotate(fn, strictDi);
      },
      has: function(name) {
        return providerCache.hasOwnProperty(name + providerSuffix) || cache.hasOwnProperty(name);
      }
 };

In this way, each injector knows your strict di information.

Also based on this changes, this line should probably be updated to angular.injector.$$annotate(blockFns[i]); in angular-mocks to bypass strict di.

Keep in mind that I don't have so much knowledge about angular internals, so I'm unsure if this could introduce some drawbacks

What do you think?

@lgalfaso
Copy link
Contributor

Would it be possible to know when calling inject.annotate is needed? Without a proper context this is getting to abstract. Now, as an answer to your proposal

  • Whatever function is exposed to annotate must not be a private API. People writing plugins like karma-jasmine should not rely on private APIs
  • Changing this behavior is a breaking change with serious consequences to every single testing plugin, and every single app that would like to at some point upgrade

This is where the context is important, as there needs to be a very good reason to make such a breaking change

@marcioj
Copy link
Author

marcioj commented Jul 27, 2015

Would it be possible to know when calling inject.annotate is needed? Without a proper context this is getting to abstract.

Sure. I changed my application to use strictDi and after fix the injector errors everything worked fine.
But I noticed that the ui-router resolve method was ignoring this config. After debugging for a while I found that annotate was being used here, making the resolve values be annotated. But because both invoke and instantiate behaves in the way I expected but annotate not, I decided to open the issue first here to know if this is either a bug in angular or not.

  • Whatever function is exposed to annotate must not be a private API. People writing plugins like karma-jasmine should not rely on private APIs
  • Changing this behavior is a breaking change with serious consequences to every single testing plugin, and every single app that would like to at some point upgrade

This is where the context is important, as there needs to be a very good reason to make such a breaking change

Agree

@lgalfaso
Copy link
Contributor

I think that the proper way would be to reopen #11728 (or create a PR with a change that implements it) and ask router-ui to use it when calling annotate

@marcioj
Copy link
Author

marcioj commented Jul 28, 2015

I liked this alternative. I'll work on it. Thanks for your help!

@marcioj
Copy link
Author

marcioj commented Jul 28, 2015

I was looking the issue tracker and found this #11734. This PR do exactly what you suggested, so I think there is no need to create another.

@Narretz
Copy link
Contributor

Narretz commented Apr 10, 2016

Handled by #11734

@Narretz Narretz closed this as completed Apr 10, 2016
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

3 participants