feat($injector): ability to load new modules after bootstrapping #16224

petebacondarwin · 2017-09-13T20:08:50Z

The new method $injector.loadNewModules(modules) will add each of the
injectables to the injector and execute all of the run blocks for each
module passed to the method.

The application developer is responsible for loading the code containing
the modules.
Modules cannot be unloaded.
Previously compiled HTML will not be affected by newly loaded directives,
filters and components.

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

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

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

Does this PR introduce a breaking change?

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:

googlebot · 2017-09-13T20:08:52Z

googlebot commented

Sep 13, 2017

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-09-13T20:08:53Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

petebacondarwin · 2017-09-13T20:10:29Z

The original commit came from #4694 and the author did sign CLA.

gkalpak

Some minor comments. LGTM overall.

gkalpak · 2017-09-14T08:32:39Z

src/auto/injector.js

+ *
+ * @description
+ *
+ * **This is an dangerous API, which you use at your own risk!**


gkalpak · 2017-09-14T08:37:51Z

src/auto/injector.js

+ * });
+ * ```
+ *
+ * @param {Module[]} mods A collection of modules to load.


AFAICT, this should be an array of strings. (We also support functions, but this seems to be an undocumented feature.)
We also use the format Array.<...> for array arguments.

gkalpak · 2017-09-14T08:47:27Z

src/auto/injector.js

+ * Add the specified modules to the current injector.
+ *
+ * This method will add each of the injectables to the injector and execute all of the run blocks
+ * for each module passed to the method.


Might (or might not) be worth also mentioning that:

Config blocks are executed as well. (Since we explicitly mention run blocks, it might sounds like config blockas are not executed.)

Loading already loaded modules has no effect (and their config/run blocks aren't executed either).

blockas -> blocks :-P

I think we should also test what happens with config blocks eh?

gkalpak · 2017-09-14T08:54:42Z

src/auto/injector.js

+ * Here is an example of loading a bundle of modules, with a utility method called `getScript`:
+ *
+ * ```javascript
+ * app.service('AmdLoader', function ($injector, $log) {


service --> factory (?)
(service works but can be confusing for beginners.)

gkalpak · 2017-09-14T09:01:32Z

src/auto/injector.js

+ *      $log.debug('AMD >> Loading Angular module [' + moduleName + '] ...');
+ *
+ *      if (config.modules) {
+ *        $injector.loadNewModules(config.modules);


Why this? Shouldn't the lazy-loaded module depend on these (as in angular.module('someModule', [...dependent modules here...])), in which case loadNewModules would automatically load them?

Not sure to be honest, this was a bit of a cut-n-paste job.

gkalpak · 2017-09-14T09:06:16Z

src/auto/injector.js

+ *      $injector.loadNewModules([moduleName]);
+ *      $log.debug('AMD >> Angular module [' + moduleName + '] loaded.');
+ *    }
+ * });


FWIW, I find the example unnecessarily complicated. I would expect something like:

app.factory('loadModule', function($injector) { return function loadModule(moduleName, bundleUrl) { return getScript(bundleUrl).then(function() { $injector.loadNewModules([moduleName]); }); }; })

gkalpak · 2017-09-14T09:10:40Z

test/auto/injectorSpec.js

+      angular.module('b', ['a'], function() { log += 'b'; }).run(function() { log += 'B'; });
+      injector.loadNewModules([
+          'b',
+          valueFn(function() { log += 'C'; }),


I would add some config fns here two 😁

gkalpak · 2017-09-14T09:12:14Z

test/auto/injectorSpec.js

+
+    it('should be able to register a service from a new module', function() {
+      var injector = createInjector([]);
+      angular.module('a');


Did you mean angular.module('a', [])?

jbedard · 2017-09-14T18:40:50Z

src/auto/injector.js

@@ -701,6 +747,11 @@ function createInjector(modulesToLoad, strictDi) {
  instanceInjector.strictDi = strictDi;
  forEach(runBlocks, function(fn) { if (fn) instanceInjector.invoke(fn); });


Worth putting this in a method to avoid duplicating it a few lines down?

In the original PR the loadNewModules was reused at application bootstrap, but this no longer works since we must modify the instanceInjector variable between loading the modules and running the run blocks.

instanceInjector = protoInstanceInjector.get('$injector');

Are you suggesting that we just make a 'executeRunBlocks` method:

function executeRunBlocks(blocks) { forEach(blocks, function(fn) { if (fn) instanceInjector.invoke(fn); }); }

which we then reuse.

It doesn't seem to save much and there is very little logic involved so I am inclined not to bother.

petebacondarwin · 2017-09-14T20:15:54Z

I have reworked following the comments.

gkalpak

One real (docs related) comment and a handful of nitpicks 😁
LGTM otherwise.

_{💯 for the added tests btw}

gkalpak · 2017-09-15T14:41:45Z

src/auto/injector.js

+ * ```javascript
+ *  app.factory('loadModule', function($injector) {
+ *  return function loadModule(moduleName, bundleUrl) {
+ *    return getScript(bundleUrl).then(function() { $injector.loadNewModules([moduleName]); });


Something got really wrong with the indentation in the three lines above 😛

gkalpak · 2017-09-15T14:43:44Z

src/auto/injector.js

+ * })
+ * ```
+ *
+ * @param {Array<String|Function|Array>=} mods an array of modules to load into the application.


Currently, the ability to pass functions/arrays as modules (e.g. in angular.module()) is undocumented. I think it makes sense to be consistent between angular.module() and loadNewModules().
(Either documenting or undocumenting in both places is fine by me.)

Also, nit: In most places we use Array.<...> (notice the dot after Array) 😇

I cut and pasted this from the angular.bootstrap() API code: https://github.com/angular/angular.js/blob/master/src/Angular.js#L1773

😞 - I am not going to block the PR for that then 😛

Then why does it say mods instead of modules? 😛

gkalpak · 2017-09-15T14:44:15Z

src/auto/injector.js

+ * @param {Array<String|Function|Array>=} mods an array of modules to load into the application.
+ *     Each item in the array should be the name of a predefined module or a (DI annotated)
+ *     function that will be invoked by the injector as a `config` block.
+ *     See: {@link angular.module modules}*


Is the trailing * intentional?

gkalpak · 2017-09-15T14:52:36Z

test/auto/injectorSpec.js

+      expect(log).toEqual(['initial', 'created', 'config1', 'config2']);
+    });
+
+    it('should execute run blocks and config blocks in the correct order', function() {


Inconsistent wording: run/config blocks (vs run/configBlocks above) 😱

gkalpak · 2017-09-15T14:53:18Z

test/auto/injectorSpec.js

+
+    it('should execute run blocks and config blocks in the correct order', function() {
+      var log = [];
+      angular.module('initial', [], function() { log.push(1)})


Missing semicolon and space inside the function. 💣

gkalpak · 2017-09-15T14:56:23Z

test/auto/injectorSpec.js

+    it('should be able to register a service from a new module', function() {
+      var injector = createInjector([]);
+      angular.module('a', []);
+      angular.module('a').factory('aService', function() {


No reason to have these two as separate lines (here and below) 😁

gkalpak · 2017-09-15T14:58:12Z

test/auto/injectorSpec.js

+      var injector = createInjector([]);
+      angular.module('a', []);
+      angular.module('a').factory('aService', function() {
+        return { sayHello: function() { return 'Hello'; } };


There should be no space after/before {/} for objects 🏄

gkalpak · 2017-09-15T14:59:43Z

test/auto/injectorSpec.js

+      });
+      injector.loadNewModules(['a']);
+      injector.invoke(function($filter) {
+        var filter = $filter('aFilter');


Save a line with injector.invoke(function(aFilterFilter) {
(Bonus points for the hidden filterFilter reference 😛)

gkalpak · 2017-09-15T15:02:34Z

test/auto/injectorSpec.js

+        var scope = $rootScope.$new();
+        var html = '<div a-directive></div>';
+        var elem = angular.element(html);
+        $compile(elem)(scope);  // compile and link


Save up to three(3) lines with:

var elem = $compile('<div a-directive></div>')($rootScope); $rootScope.$digest();

🌿

gkalpak · 2017-09-15T15:03:24Z

test/auto/injectorSpec.js

+      var injector = createInjector(['ng']);
+      angular.module('a', []);
+      angular.module('a').directive('aDirective', function() {
+        return {  template: 'test directive' };


Way too much whitespace inside object 😇

gkalpak

LGTM (except for a couple of indentation issues - including #16224 (comment) 😁)

gkalpak · 2017-09-18T14:57:04Z

test/auto/injectorSpec.js

+      var injector = createInjector(['initial']);
+      log.push('created');
+
+      angular.module('a', [], function() { log.push(4);})


Missing whitespace after ; 🔢

Narretz

I assume one of the common use cases is to load additional modules when the user navigates inside the application, i.e. before a route is loaded.

This means the application must download the module and load it, but only for the first time (as the modules cannot be unloaded).

I don't see a mechanism for checking if a module has been loaded already. We don't expose the list in the injector, either. Should we add a convenience method hasModule() ?
You could always check if a specific service is in the DI, but this won't work for modules that only load components etc.
It's not enough that the injector won't load a module twice either, it's the download that must be avoided. Correct caching could handle that ofc.

Georgios pointed out that we have https://docs.angularjs.org/api/auto/service/$injector#modules for that

Narretz · 2017-09-18T15:27:17Z

src/auto/injector.js

+ * ```javascript
+ *  app.factory('loadModule', function($injector) {
+ *  return function loadModule(moduleName, bundleUrl) {
+ *    return getScript(bundleUrl).then(function() { $injector.loadNewModules([moduleName]); });


You could use fetch in-place here for getScript. Only thing that might happen is that people use it and complain that it doesn't work in IE9 ...

@Narretz I think the difference is that getScript also executes the script.

Narretz · 2017-09-18T15:29:21Z

src/auto/injector.js

+ * })
+ * ```
+ *
+ * @param {Array<String|Function|Array>=} mods an array of modules to load into the application.


Then why does it say mods instead of modules? 😛

gkalpak · 2017-09-18T15:57:03Z

I don't see a mechanism for checking if a module has been loaded already. We don't expose the list in the injector, either.

Doesn't $injector.modules cover that?

Narretz · 2017-09-18T16:11:26Z

It does, thanks. I forgot we added that. Maybe we could add a note about the property to the docs.

petebacondarwin · 2017-09-18T18:18:54Z

@Narretz do you mean add a link to the modules property docs in the docs for the loadNewModules method?

Narretz · 2017-09-18T18:28:31Z

@petebacondarwin that's right

The new method `$injector.loadNewModules(modules)` will add each of the injectables to the injector and execute all of the config and run blocks for each module passed to the method. * The application developer is responsible for loading the code containing the modules. * Modules cannot be unloaded. * Previously loaded modules will not be reloaded. * Previously compiled HTML will not be affected by newly loaded directives, filters and components.

petebacondarwin · 2017-09-21T12:54:53Z

Cherry picked to 1.6.x as 6e78fee

petebacondarwin added branch: 1.6.x cla: yes component: di frequency: low needs: review severity: inconvenient type: feature labels Sep 13, 2017

googlebot added cla: no and removed cla: yes labels Sep 13, 2017

petebacondarwin mentioned this pull request Sep 13, 2017

feat($injector): ability to load new modules after bootstrapping #4694

Closed

petebacondarwin added cla: yes and removed cla: no labels Sep 13, 2017

gkalpak suggested changes Sep 14, 2017

View reviewed changes

jbedard reviewed Sep 14, 2017

View reviewed changes

googlebot added cla: no and removed cla: yes labels Sep 14, 2017

petebacondarwin force-pushed the module-loading-after-bootstrap branch from 8a44f9c to b1c8a43 Compare September 14, 2017 20:16

gkalpak approved these changes Sep 15, 2017

View reviewed changes

petebacondarwin force-pushed the module-loading-after-bootstrap branch from b1c8a43 to 747384a Compare September 16, 2017 19:18

gkalpak approved these changes Sep 18, 2017

View reviewed changes

Narretz reviewed Sep 18, 2017

View reviewed changes

petebacondarwin force-pushed the module-loading-after-bootstrap branch from 747384a to 860f9e9 Compare September 18, 2017 21:14

petebacondarwin force-pushed the module-loading-after-bootstrap branch from 860f9e9 to 8186065 Compare September 21, 2017 12:52

petebacondarwin merged commit 34237f9 into angular:master Sep 21, 2017

		@@ -701,6 +747,11 @@ function createInjector(modulesToLoad, strictDi) {
		instanceInjector.strictDi = strictDi;
		forEach(runBlocks, function(fn) { if (fn) instanceInjector.invoke(fn); });

feat($injector): ability to load new modules after bootstrapping #16224

feat($injector): ability to load new modules after bootstrapping #16224

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!