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

feat(injector): "strict-DI" mode which disables "automatic" function annotation / inferred dependencies #6719

Closed
wants to merge 1 commit into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Mar 17, 2014

This modifies the injector to prevent automatic annotation (or inferring dependencies based on formal parameter names) from occurring for a given injector.

This behaviour can be enabled when bootstrapping the application by using the attribute
"ng-strict-di" on the root element (the element containing "ng-app"), or alternatively by passing
an object with the property "strictDi" set to "true" in angular.bootstrap, when bootstrapping
manually.

JS example:

    angular.module("name", ["dependencies", "otherdeps"])
      .provider("$willBreak", function() {
        this.$get = function($rootScope) {
        };
      })
      .run(["$willBreak", function($willBreak) {
        // This block will never run because the noMagic flag was set to true,
        // and the $willBreak '$get' function does not have an explicit
        // annotation.
      }]);

    angular.bootstrap(document, ["name"], {
      strictDi: true
    });

HTML:

    <html ng-app="name" ng-strict-di>
      <!-- ... -->
    </html>

This will only affect functions with an arity greater than 0, and without an $inject property.

@btford btford added this to the 1.3.0 milestone Mar 17, 2014
@auser auser self-assigned this Mar 17, 2014
@caitp caitp added cla: yes and removed cla: no labels Mar 18, 2014
@btford
Copy link
Contributor

btford commented Mar 25, 2014

I like this a lot.

@tbosch tbosch assigned btford and unassigned auser Mar 31, 2014
@tbosch tbosch modified the milestones: 1.3.0-beta.5, 1.3.0 Mar 31, 2014
@caitp
Copy link
Contributor Author

caitp commented Mar 31, 2014

So, per discussion today, we should do this in bootstrap and the ng-app directives instead of making this change in the module definition layer

The problem becomes testing it, since angular-mocks doesn't really "bootstrap" anything, per se.

I think keeping the module API as is (in the patch), but removing the public API from it (no extra param to angular.module()), and then just adjusting angular-mocks and the bootstrap/auto-bootstrap code should be good enough

*
<example>
<file name="index.html">
<div ng-app-strict="ngAppStrictDemo">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petebacondarwin I'd like to use this in an example, but the docs preprocessor only understands the ng-app attribute and not ng-app-strict (if that does continue to be the correct name).

I was thinking it wouldn't be so bad to change the dgeni-packages template so that if ng-app-strict is present, it will include -strict, but I dunno.

In any case, this makes things kind of awkward.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not convinced that controlling this with a directive is the best API. If you are creating an application that is complex enough that you will need this then you are going to be writing a bunch of modules in JavaScript, in which case it is not a big deal to apply the flag to the module. For instance, you could have a method on the module API:

angular.module('myApp', ['ngRoute'])
  .requireAnnotations()
  .controller(...)
  ...
  .factory(...);

This is also more self-documenting - I doubt that anyone would ever be able to guess what ng-app-strict actually meant, without having to go to the docs.

Any, regarding the examples, we have a nice feature that asks Dgeni not to automatically apply an ng-app to the index.html wrapper that gets generated, which allows us to apply it explicitly inside the example itself. We use this in the Concepts guide: https://github.com/angular/angular.js/blob/master/docs/content/guide/concepts.ngdoc#L35.

In this case the example will look like:

<example ng-app-included="true">
   <file name="index.html">
   <div ng-app-strict="ngAppStrictDemo">
   ...
   </file>
</example>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance, you could have a method on the module API:

This was discussed during the meeting, the problem with doing this at the module level is that it technically applies to all modules in the application, even if it's added by a third-party one. Doing this during bootstrapping means that it's in the application's control, regardless of what third party modules are doing.

I do agree that the directive name could (should) be better, but I didn't want to spend too much time thinking about what it should be.


Onto the docs thing (again), ah, I thought the docs processor was preprocessing it to find the attribute, but I guess not. So that should fix that up.

@jeffbcross jeffbcross modified the milestones: 1.3.0-beta.6, 1.3.0-beta.5 Apr 3, 2014
@IgorMinar
Copy link
Contributor

can we:

  • rename this to strictDi and ng-strict-di
  • add an error page

@caitp
Copy link
Contributor Author

caitp commented Apr 7, 2014

@btford
Copy link
Contributor

btford commented Apr 7, 2014

Looks good to me!

@@ -0,0 +1,55 @@
@ngdoc error
@name $injector:strctdi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strctdi is a bit vague - I might think strct is to do with structure? Is there a problem with using strictdi?

var attr, i, ii = ngAttrPrefixes.length, j, jj;
for (i=0; i<ii; ++i) {
attr = ngAttrPrefixes[i] + ngAttr;
if (element.getAttribute) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we sniff whether getAttribute is available outside this loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's less work to just defer to jqLite for this, since jquery/jqlite should have already been chosen by the time this is called. Testing locally it works, so it's probably okay.

@IgorMinar
Copy link
Contributor

lgtm. just squash it into a single commit please

…annotation

This modifies the injector to prevent automatic annotation from occurring for a given injector.

This behaviour can be enabled when bootstrapping the application by using the attribute
"ng-strict-di" on the root element (the element containing "ng-app"), or alternatively by passing
an object with the property "strictDi" set to "true" in angular.bootstrap, when bootstrapping
manually.

JS example:

    angular.module("name", ["dependencies", "otherdeps"])
      .provider("$willBreak", function() {
        this.$get = function($rootScope) {
        };
      })
      .run(["$willBreak", function($willBreak) {
        // This block will never run because the noMagic flag was set to true,
        // and the $willBreak '$get' function does not have an explicit
        // annotation.
      }]);

    angular.bootstrap(document, ["name"], {
      strictDi: true
    });

HTML:

    <html ng-app="name" ng-strict-di>
      <!-- ... -->
    </html>

This will only affect functions with an arity greater than 0, and without an $inject property.

Closes angular#6719
Closes angular#6717
Closes angular#4504
Closes angular#6069
Closes angular#3611
@caitp caitp closed this in 4b1695e Apr 10, 2014
@btford
Copy link
Contributor

btford commented Apr 10, 2014

🎊

@caitp caitp changed the title feat(injector): optionally disable automatic function annotation feat(injector): "strict-DI" mode which disables "automatic" function annotation / inferred dependencies Apr 10, 2014
caitp added a commit to caitp/angular.js that referenced this pull request Apr 11, 2014
…annotation

This modifies the injector to prevent automatic annotation from occurring for a given injector.

This behaviour can be enabled when bootstrapping the application by using the attribute
"ng-strict-di" on the root element (the element containing "ng-app"), or alternatively by passing
an object with the property "strictDi" set to "true" in angular.bootstrap, when bootstrapping
manually.

JS example:

    angular.module("name", ["dependencies", "otherdeps"])
      .provider("$willBreak", function() {
        this.$get = function($rootScope) {
        };
      })
      .run(["$willBreak", function($willBreak) {
        // This block will never run because the noMagic flag was set to true,
        // and the $willBreak '$get' function does not have an explicit
        // annotation.
      }]);

    angular.bootstrap(document, ["name"], {
      strictDi: true
    });

HTML:

    <html ng-app="name" ng-strict-di>
      <!-- ... -->
    </html>

This will only affect functions with an arity greater than 0, and without an $inject property.

Closes angular#6719
Closes angular#6717
Closes angular#4504
Closes angular#6069
Closes angular#3611
caitp added a commit that referenced this pull request Apr 11, 2014
…annotation

This modifies the injector to prevent automatic annotation from occurring for a given injector.

This behaviour can be enabled when bootstrapping the application by using the attribute
"ng-strict-di" on the root element (the element containing "ng-app"), or alternatively by passing
an object with the property "strictDi" set to "true" in angular.bootstrap, when bootstrapping
manually.

JS example:

    angular.module("name", ["dependencies", "otherdeps"])
      .provider("$willBreak", function() {
        this.$get = function($rootScope) {
        };
      })
      .run(["$willBreak", function($willBreak) {
        // This block will never run because the noMagic flag was set to true,
        // and the $willBreak '$get' function does not have an explicit
        // annotation.
      }]);

    angular.bootstrap(document, ["name"], {
      strictDi: true
    });

HTML:

    <html ng-app="name" ng-strict-di>
      <!-- ... -->
    </html>

This will only affect functions with an arity greater than 0, and without an $inject property.

Closes #6719
Closes #6717
Closes #4504
Closes #6069
Closes #3611
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants