Module info #15225

petebacondarwin · 2016-10-07T13:03:29Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

feature

What is the current behavior? (You can also link to an open issue here)

No way to find information about the currently loaded modules

What is the new behavior (if this is a feature change)?

Two new features:

a method on Module (info()) that lets developers add arbitrary info about their modules.
a property on $injector (modules) that lets developers access the module objects that were loaded into the injector at bootstrap.

This second item could also be useful to @ocombe for his ocLazyLoad project.

Does this PR introduce a breaking change?

No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

Replaces the previous #12465 PR

ocombe · 2016-10-07T13:43:46Z

It's happening !!!!

gkalpak · 2016-10-07T14:20:09Z

src/loader.js

+           * {@link $injector#modules `$injector.modules`} property:
+           *
+           * ```js
+           * var version = $injector.modules['myModule'].info().version;


Wouldn't angular.module('myModule').info() also work?

Yes, but in that case you do know, necessarily if the module was
a) loaded in the first place
b) added to the injector
If it was not loaded then your line of code would throw an exception

I mean use it in place of $injector.modules['myModule'].info() - i.e. at "runtime" - so it should have been added to the injector.
I was mainly curious basically. $injector.modules['myModule'] works for me 😃

Hm...since we brought it up, $injector is kind of ambiguous, since it may refer to the providerInjector (which doesn't know about modules) or the instanceInjector (which does).

I doubt anyone is using the providerInjector, but still it might be a good idea to add modules on the providerInjector as well (for symmetry).

I don't follow. You can't use angular.module('myModule').info() since you don't know if that module has been loaded or included in the current app.
In fact, you don't even know what the current app is since there might be more than one bootstrapped on a page.

Also, $injector is not ambiguous. It is the only injector that you can inject into components and services, etc.

But maybe I'm missing the point...

I think he's referring to the $injector that you can inject into config function which is not the same injector as is injected into a service, component, etc.

Yes, I am referring to the providerInjector that can be injected into providers/config blocks. E.g.:

.provider('foo', ['$injector', function fooProvider(providerInjector) { this.$get = ['$injector', function foo(instanceInjector) { console.log(providerInjector === instanceInjector); // false } })

(Again, I don't expect many people to be actually using it, but you never know 😃)

Regarding angular.module('someModule').info():
Assume you are the author of a library or addon that relies on another library (e.g. myAwesomeMaterialToolbar). You can request that your addon is loaded after ngMaterial, so you can detect its version and do different things. E.g.:

// my-awesome-material-toolbar.js var mdVersion = angular.module('ngMaterial').info().version; var deps = (mdVersion >= '1.1.1') ? [] : ['myShimForAFeatureAddedInNgMaterial1.1.1']; angular.module('myAwesomeMaterialToolbar', deps);

I will fix the providerInjector/instanceInjector thing.

Regarding the load time dependencies use case. For sure you can do what you suggest with the functionality provided in this PR, although you are having to ensure that the developer loads the script files in the correct order and is not using some async loading helper that might mess things up.

It would be safer (IMO) to do configuration of your awesome library in providers, etc... Although I accept it is difficult to optionally add in a directive, you could make the directive a noop if not wanted.

Works for me 👍

gkalpak · 2016-10-07T14:36:16Z

src/loader.js

+           *                          or `this` if called as a setter.
+           *
+           * @description
+           * Additional info about this module


This makes it sound as if passing an object would extend the existing info (not overwrite it).
I think we should be pretty explicit about that.

gkalpak · 2016-10-07T14:38:57Z

test/loaderSpec.js

+
+      it('should store the object passed as a param', function() {
+        theModule.info({ version: '1.2' });
+        expect(theModule.info()).toEqual({ version: '1.2' });


I would add a test to verify it overwrites the previous info object.

petebacondarwin · 2016-10-08T15:18:02Z

PTAL @gkalpak

gkalpak · 2016-10-08T17:33:00Z

src/loader.js

+           */
+          info: function(value) {
+            if (isDefined(value)) {
+              if (!isObject(value)) throw ngMinErr('aobj', 'Argument \'{0}\' must be an object', 'value');


This should probably be value instead of 'value' (i.e. without the quotes), right?

No. I don't think so. The idea is that the error can be general and be used with other parameters names

Right. Then why do you need the {0}? Why don't you just write 'Argument \'value\' must be an object' directly? I guess in order to make it easier to recognize the part that is interchangeable.
👍

I am not 100% sure but I think that the format string has to be consistent for all usage of the error.

gkalpak · 2016-10-08T17:34:15Z

test/auto/injectorSpec.js

+      var test1 = angular.module('test1', ['test2']).info({ version: '1.1' });
+      var test2 = angular.module('test2', []).info({ version: '1.2' });
+      module('test1');
+      inject(['$injector', function($injector) {


OOC, is the array necessary?

Didn't we decide to make all our tests minification proof?

I'm sure there was an issue about it

I don't remember anything like that (why would we)?
Maybe we have decided to make ngMocks minification-safe (because it makes it easier for some people to incorporate it into their workflow), but not our tests.

Ah, yes you are right @gkalpak (as always) - I was thinking of this: #13542

They don't have to be minification-proof but shouldn't we aim at making them pass in strict DI mode?

There is not such thing as "strict DI" in tests :)
(Also none of the other tests are, so it is obviously not a problem.)

There is not such thing as "strict DI" in tests :)

What I meant by that is that we are manually annotating the injected functions, if strict DI is enabled.

gkalpak · 2016-10-10T07:07:49Z

LGTM _{(with or without the array notation 😃)}

petebacondarwin · 2016-10-10T07:18:03Z

Thanks @gkalpak - I just thought we should check that there is something similar in A2 before landing this.

petebacondarwin · 2017-02-27T20:50:24Z

So in Angular there is the NgModule concept, which provides a similar place to store useful metadata about your modules/app. Let's merge this.

gkalpak

Have you considered "version-stamping" ngLocale?

gkalpak · 2017-02-27T22:06:26Z

src/AngularPublic.js

@@ -265,5 +265,6 @@ function publishExternalAPI(angular) {
        $$cookieReader: $$CookieReaderProvider
      });
    }
-  ]);
+  ])
+  .info({ angularVersion: '"NG_VERSION_FULL"' });


Why not call it just version (since it's already namespaced by the module name).
Especially in other modules (ngAnimate, ngAria, etc).

I guess I was leaning towards the idea that you would stamp your module with the version of Angular that it was designed for, rather than the version of the module itself...

To me angularVersion seems more intuitive; otherwise I'd feel we're talking about the module version as Pete said.

I tend to think it as the module version indeed. But it doesn't make any real difference. angularVersion works for me.

petebacondarwin · 2017-02-28T12:56:19Z

I will add version stamping to the locale modules too.

petebacondarwin · 2017-02-28T21:52:10Z

It turns out that adding the version stamp to the locale files is a little more involved with changes to grunt required :-(

gkalpak · 2017-03-01T10:28:29Z

Sounds like it's not worth it. It still LGTM without vervion-stamping locale files 😃

The new `info()` method lets developers store arbitrary information about their module for consumption later.

The `modules` property is a hash of the modules loaded into the injector at bootstrap time. This can be used to access the module's info.

You can now check what version of AngularJS a core module is designed for: ``` var angularVersion = $injector.modules['myModule'].info().angularVersion; ```

The new `info()` method lets developers store arbitrary information about their module for consumption later. Closes #15225

The new `info()` method lets developers store arbitrary information about their module for consumption later. Closes angular#15225

googlebot added the cla: yes label Oct 7, 2016

petebacondarwin mentioned this pull request Oct 7, 2016

feat(angular.info): optionally store and access additional info on mo… #12465

Closed

petebacondarwin added type: feature frequency: low needs: review component: misc core needs: public api severity: inconvenient branch: 1.5.x labels Oct 7, 2016

petebacondarwin added this to the 1.6.x (post 1.6.0) milestone Oct 7, 2016

gkalpak reviewed Oct 7, 2016

View reviewed changes

petebacondarwin force-pushed the module-info branch from 8a38763 to 8c4dd97 Compare October 8, 2016 15:17

gkalpak suggested changes Oct 8, 2016

View reviewed changes

gkalpak approved these changes Oct 10, 2016

View reviewed changes

petebacondarwin modified the milestones: 1.6.3, 1.6.x Feb 8, 2017

petebacondarwin force-pushed the module-info branch from 8c4dd97 to 8a12a75 Compare February 27, 2017 20:54

gkalpak reviewed Feb 27, 2017

View reviewed changes

petebacondarwin force-pushed the module-info branch 2 times, most recently from 972e0a2 to 13326ee Compare February 28, 2017 12:26

feat(Module): add info() method

3490d5b

The new `info()` method lets developers store arbitrary information about their module for consumption later.

petebacondarwin added 2 commits March 2, 2017 08:20

feat($injector): add new modules property

8e7a652

The `modules` property is a hash of the modules loaded into the injector at bootstrap time. This can be used to access the module's info.

feat(info): add angularVersion info to each module

2a5a410

You can now check what version of AngularJS a core module is designed for: ``` var angularVersion = $injector.modules['myModule'].info().angularVersion; ```

petebacondarwin force-pushed the module-info branch from 13326ee to 2a5a410 Compare March 2, 2017 08:21

petebacondarwin closed this in 49aba51 Mar 2, 2017

petebacondarwin added a commit that referenced this pull request Mar 2, 2017

feat(Module): add info() method

09ba690

The new `info()` method lets developers store arbitrary information about their module for consumption later. Closes #15225

ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017

feat(Module): add info() method

feb3481

The new `info()` method lets developers store arbitrary information about their module for consumption later. Closes angular#15225

Module info #15225

Module info #15225

Conversation

petebacondarwin commented Oct 7, 2016

ocombe commented Oct 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Oct 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Oct 10, 2016

petebacondarwin commented Oct 10, 2016

petebacondarwin commented Feb 27, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Feb 28, 2017

petebacondarwin commented Feb 28, 2017

gkalpak commented Mar 1, 2017

gkalpak Oct 9, 2016 •

edited

Loading

gkalpak Oct 9, 2016 •

edited

Loading