Creating a module twice fails silently

[amirhyoussefi]

angular.module('foo', []).directive('bar', ...);
angular.module('foo', []).filter('baz', ...);

will cause the first directive registration to be lost.

[petebacondarwin]

This is by design - you should be able to replace a module with a new one, particularly useful in creating test mocks.

This is one reason why I tend to use exactly one module to one file.

I agree though that this is an easy mistake to make and perhaps there should be a more explicit API, perhaps:

angular.module -> creates a new module and errors if the module name already exists angular.replaceModule -> replaces a module with the given one and errors if the module name does not already exist angular.addToModule -> reopens a module to add components to it; errors if the module name does not already exist. (This is equivalent to the current angular,module without providing dependencies)

[nahue]

+1 i was having this problem and i didnt know why :)

[ifitzpatrick]

+1 for a more explicit api

[geddski]

@amirhyoussefi you'd be better off creating a module in one file, say foo.js and adding things to it in other files. Use angular.module() with just one parameter and it will fetch the already created module rather than overriding it. For example:

foo.js

angular.module('foo', []); 

bar.js

angular.module('foo').directive('bar', ...);

baz.js

angular.module('foo').filter('baz', ...);

This pattern also has the benefit of not needing to introduce another global variable for your module. Yay.

[amirhyoussefi]

If we can provide a warning when it happens that would be great. An IDE can flag this (to say the least).

@nah I had the same problem as the issue was hard to debug.

[anjo]

Why not simply add to the dependencies? I.e. you would have:

angular.module("foo", null, [some new deps])

and use null as a flag to add to the deps instead of replacing. The way it is now it's really a bitch to find out where the problem is when you suddenly get a blank page and nothing works

[anjo]

@geddesign but the downside is that you need to remember to init your module in advance of calling angular.module('foo'). You won't really run into an error, but things won't work when you test.

Another downside is that you can't add to the dependencies when you actually declare them, ie. when you have a larger module, you might want to define your dependency with your service and not with the module:

angular.module("foo",["stuff this service depends on"]).service(...)

so it will get added to later on.

[qrow]

I run into this problem and could'n understand what i did wrong, there were no error and app just stopped working because i override whole module together with routes.

This is bad experience for people that starting with angular as information about possibility to override module is hidden in documentation in one of parameters description.

If there is no plans to change API, then maybe warning could be written to console, because i think this will be common mistake for beginners.

Also documentation of module function if wrong in "Returns" part:

Returns {module} – new module with the angular.Module api.

because it returns new module only if you specify dependencies, in other case it returns existing module.

[btford]

As part of our effort to clean out old issues, this issue is being automatically closed since it has been inactivite for over two months.

Please try the newest versions of Angular (1.0.8 and 1.2.0-rc.1), and if the issue persists, comment below so we can discuss it.

Thanks!

[iven]

It wastes me half a day to find this issue page. I could just get useless error messages like: 'unknown provider', or 'circular dependency'.

You should really consider using new APIs like:

var newFooModule = angular.module.create('foo');
var existingFooModule = angular.module.get('foo');

or provide a warning message in the console.

BTW, I'm using 1.2.0-rc2.

[petebacondarwin]

I agree On 12 Oct 2013 09:21, "Iven" notifications@github.com wrote:

It wastes me half a day to find this issue page. I could just get useless error messages like: 'unknown provider', or 'circular dependency'.

You should really consider using new APIs like:

var newFooModule = angular.module.create('foo'); var existingFooModule = angular.module.get('foo');

or provide a warning message in the console.

BTW, I'm using 1.2.0-rc2.

— Reply to this email directly or view it on GitHubhttps://github.com/angular/angular.js/issues/1779#issuecomment-26193471 .

[hiddentao]

I find that there are two fundamental requirements that the current angular.module API doesn't easily satisfy:

1. Being able to split up a module into multiple files so that I can keep a clean codebase, without having to worry about making sure I call angular.module(name, deps) before I call it without the deps parameter.
2. Being able to easily add dependencies to an already created module (something that follows on from the previous point, since I'd like to list dependencies in the context in which they are being used).

I wrote a decorator for angular.module which satisfies the above two requirements, but then breaks the design requirement that an existing module can be completely overridden (e.g. for testing).

Looking at the API we could change the single-argument version of angular.module() such that it automatically creates the module if it doesn't already exist but then this breaks a bunch of tests which check that invalid module names result in error thrown during the app initialisation process.

So for now I propose just adding a new API which makes it easy to append to the list of dependencies:

var mod = angular.module('name');
mod.dependencies(['dep1', 'dep2']);

I'll work on raising a pull request for this.

[j-walker23]

@hiddentao if you split up everything into separate modules and import them into their direct parents then the current api works just fine and satisfies all problems. That is the way i went after trying to declare parts of modules in separate files. Just have separate modules in separate files, and import when needed. One module per file.

[hiddentao]

@j-walker23 Yeah that would work. But I'd like to be able to do it the way I want as splitting a module into multiple files just intuitively makes sense to me; it's what I'm used to from other frameworks. I also stick to one module per file..I guess you meant to say one file per module?

[j-walker23]

I understand, its hard to move away from a pattern that you like and are used to. The other reason i had to stop partial module declarations was my build script. To load a partial module the original declaration had to be imported first, and everything was imported alphabetical due to grunt reading the directory. If you have not, check out https://github.com/ngbp/ngbp. It helped me figure out a good module structure for a large app.

Sorry, yeah i meant one file per module.

[bahmutov]

I wrote a small proxy script in https://github.com/bahmutov/stop-angular-overrides. Install using bower install stop-angular-overrides, load after angular but before modules. Checks if .module, .filter or .controller with same name already exists before passing to actual angular function. If exists, throws an exception.

[amio]

How about making angular.module('anUndefinedModule') returns undefined instead of throwing out an Error? Then we can do something like this:

(angular.module('anUndefinedModule') || angular.module('anUndefinedModule', []))
  .filter(/*...*/)

[acrodrig]

+1 on last comment. Just spent half a day chasing this issue (and arriving at this page).

[mgol]

@petebacondarwin, I hope you don't mind me reopening this issue for now. I think even though current behavior is needed for Karma we could have current behavior available only in Karma, via switching a particular config flag in angular.mock. Let's evaluate it, a lot of people (including me) have been bitten by that one.

[petebacondarwin]

Perhaps this is something that we can fix in 1.4?

[mgol]

@petebacondarwin That would be great!

[mgol]

@petebacondarwin Should we have a milestone for things we plan for 1.4?

[petebacondarwin]

We have the "1.4 candidate" label at the moment

[cironunes]

I would like to send a PR for this. @petebacondarwin what do you think that should be done? Extend the API with replaceModule and addToModule?

[petebacondarwin]

We need to be careful about this. How about trying to define a module that already exists throws and add a new method removeModule? Thoughts @mzgol et al

[petebacondarwin]

This would be mostly backwardly compatible

[lgalfaso]

maybe all is needed is adding a method createOrUpdate(moduleName:string, dependencies:Array) that would do the following:

• If there is no module named moduleName create it and use dependencies as the dependencies
• If there is a module names moduleName then retrieve it, add the dependencies to the module dependencies and return it

This way (as long as the application is not started), you can keep on adding dependencies to a module and get the module in a safe way

[shahata]

@lgalfaso hmmm, you could do that today with angular.module('moduleName').requires.push(deps)

I'm thinking that it is better to throw in case of double definition (both for module and injectables), but maybe only do it in strictDi mode?

[lgalfaso]

@shahata the idea is that if you have 10 files that compose a module, then each can do

angular.createOrUpdate('myModule', [/* my dependencies */])

and od not care about the order.

We cannot just throw with double definitions is used extensivelly in testing

[shahata]

Yeah, of course. angular.mock.module does not need to throw, only stuff defined through angular.module.

[shahata]

createOrUpdate might be a good idea, I don't know. I'm not sure for how many apps this kind of split requirements makes sense.

[j-walker23]

Hey guys. Came across this and am curious. In the last couple of years i have never come across this bug or issue. But it seems that its pretty important and a lot of devs run into it. What I am curious about is if there is some good design pattern or something like that that i am unaware of? Especially if the pattern i use has flaws or could be made better and by understand what you guys do that sometimes causes this issue.

My current pattern is every file is a separate module. If i have 5 or 10 files that are all related then i would have a "parent" module and the rest would get imported into the parent. So thats really up to 11 different modules, but only the parent is imported by my actual applications.

Are there any performance issues with that pattern that could be helped out by this issue? Or if its important for another reason it would be nice to understand. Thanks for any help, and good luck!

[caitp]

There's basically no reason why we don't throw. It would be a small patch to make it work

[petebacondarwin]

I think in 1.4 we should throw in this scenario:

angular.module('someModule', []);
angular.module('someModule', []);  // <- throw an error here

This way we don't have the current problem where we silently overwrite everything that was there already.

But this leaves us with a question of how to safely override a module. We have a few options:

angular.replaceModule('someModule', []);

angular.removeModule('someModule');
angular.module('someModule', []);

The other question of how to safely extend a module is separate to the current problem.

[pkozlowski-opensource]

@petebacondarwin do we want to support overriding of modules?

[caitp]

what is the use case? we don't really need it for testing since you'd just create a new injector for each test --- and I'm not sure what the value is in production (re: removing or replacing existing modules)

[petebacondarwin]

I guess you are right. I can't think of a good use case. Anyone else?

[lgalfaso]

@caitp if you put a breakpoint at https://github.com/angular/angular.js/blob/master/src/loader.js#L90 and run the test, then you will see that we hit this line a lot

[caitp]

@lgalfaso I think those are arguably bugs in our tests, there's no good reason for it --- especially since tests are mostly using anonymous modules (entirely I think, except for the modules tests + ng, ngLocale, ngAnimate, maybe ngAria)

[shahata]

How about throwing only in strictDi mode?

[pkozlowski-opensource]

@shahata before deciding to throw only in the strictDi mode, could we please list valid use-cases where people might want to override modules? For me it was always pure bug, I can't think of a valid scenario (disclaimer: I'm not saying there is none, just saying that I can't see any so let's list them and then weight / decide).

[shahata]

@pkozlowski-opensource I guess it mostly happens in the real world by mistake - including some js file twice by mistake or including multiple versions of some angular library one after the other. I don't know if someone will do this on purpose but I guess there got be some hackish reasons one would want to replace a whole module without removing the overwritten version from the document.

[WeHateNick]

Would anyone be kind enough to suggest an easy way to find all the instances where a module is overwritten to fix this issue?

I tried doing a search for "angular.module('vitalsApp', [" (escaping RegEx characters) in my project, and only found one instance of it. Could I be missing it somehow or do you think I have a different issue?

I get an $injector:unpr error from Karma, so I figured I was overwriting the module somewhere.

[petebacondarwin]

You could redefine angular.module:

var originalModule = angular.module;
var modules = [];
angular.module = function(name, deps) {
  if ( deps && modules.indexOf(name) !== -1 ) {
    throw new Error('redefining module: ' + name);
  }
  modules.push(name);
  console.log(modules);
  return originalModule(name, deps);
};

See http://plnkr.co/edit/i47L3cR1Tiylvvy4f9Ko?p=preview

But your problem is probably elsewhere. The error message should identify the bad service/provider. It could be also that your karma missing is missing a source file?

[bahmutov]

@WeHateNick @petebacondarwin you can include https://github.com/bahmutov/stop-angular-overrides script before your client code to overwrite module (and other calls) and stop silent behavior

[btford]

I'm going to be working on this for Angular 1.4

[ocombe]

@pkozlowski-opensource in case of lazy loading files (with ocLazyLoad for example) you might want to call the same file twice to re-execute run/config blocks. I'm not saying that it's good practice, but it can happen.

Also if your module is generated server-side with a few key elements changing, you might want to redefine the modules (ie: when the user logs in).

It depends on how you implement the "fix", could you maybe add an option to keep the current implementation (like the "debugInfoEnabled" option ?), it would throw the error by default but you could set a flag somewhere to say: ok I know it's risky but let me do it.

Also if you could add at the same time an option to list the currently loaded modules & components (directives, services, ...), this would be really awesome for lazy loading libraries (I think @geddski would like that for Overmind as well). I know that you don't support lazy loading components in angular 1.x, but I don't think it would be that hard to do: the list exists internally, it's just a matter of making it available via a method (probably on the $injector, I could send a PR for that if you want). If you add this option, I think that I could work on a library to make ES6 angular 2.x modules work with angular 1.x (the "import" part included).

[pkozlowski-opensource]

@ocombe what you are saying is true but it uses on-demand / lazy loading as a main argument. So IMO we should focus on making lazy-loading the right way.

[ocombe]

I agree, and it would be fantastic to be able to do that for angular 1.x, but it will be a lot of work. It would also make the transition from 1.x to 2.x much smoother and that's something that you are looking for.

If you decide to work on a real lazy loading for angular, let me know I'd be glad to help you !

[chrisadams]

I've wasted maybe 4-5 hours trying to solve a generic Error: [$injector:unpr] Unknown provider: ____Provider <- _____ error today because of this. A warning would be great.

I just assumed reopening would add to the module definition rather than replacing it. Clearly this was a mistake, but it's an easy mistake to make and it's difficult to determine the cause of your error.