New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creating a module twice fails silently #1779

Open
amirhyoussefi opened this Issue Jan 9, 2013 · 52 comments

Comments

Projects
None yet
@amirhyoussefi

amirhyoussefi commented Jan 9, 2013

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

will cause the first directive registration to be lost.

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Jan 9, 2013

Member

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)

Member

petebacondarwin commented Jan 9, 2013

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

This comment has been minimized.

Show comment
Hide comment
@nahue

nahue Jan 11, 2013

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

nahue commented Jan 11, 2013

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

@ifitzpatrick

This comment has been minimized.

Show comment
Hide comment
@ifitzpatrick

ifitzpatrick Jan 11, 2013

+1 for a more explicit api

ifitzpatrick commented Jan 11, 2013

+1 for a more explicit api

@geddski

This comment has been minimized.

Show comment
Hide comment
@geddski

geddski Jan 16, 2013

Contributor

@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.

Contributor

geddski commented Jan 16, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@amirhyoussefi

amirhyoussefi Jan 16, 2013

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.

amirhyoussefi commented Jan 16, 2013

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

This comment has been minimized.

Show comment
Hide comment
@anjo

anjo Feb 11, 2013

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 commented Feb 11, 2013

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

This comment has been minimized.

Show comment
Hide comment
@anjo

anjo Feb 11, 2013

@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.

anjo commented Feb 11, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@qrow

qrow Mar 4, 2013

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.

qrow commented Mar 4, 2013

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 btford closed this Aug 24, 2013

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Aug 24, 2013

Contributor

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!

Contributor

btford commented Aug 24, 2013

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

This comment has been minimized.

Show comment
Hide comment
@iven

iven Oct 12, 2013

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.

iven commented Oct 12, 2013

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

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Oct 12, 2013

Member

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
.

Member

petebacondarwin commented Oct 12, 2013

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 added a commit to hiddentao/angular.js that referenced this issue Jan 20, 2014

@hiddentao

This comment has been minimized.

Show comment
Hide comment
@hiddentao

hiddentao Jan 20, 2014

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.

hiddentao commented Jan 20, 2014

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

This comment has been minimized.

Show comment
Hide comment
@j-walker23

j-walker23 Mar 25, 2014

@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.

j-walker23 commented Mar 25, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@hiddentao

hiddentao Mar 26, 2014

@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?

hiddentao commented Mar 26, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@j-walker23

j-walker23 Mar 26, 2014

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.

j-walker23 commented Mar 26, 2014

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

This comment has been minimized.

Show comment
Hide comment
@bahmutov

bahmutov Apr 11, 2014

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.

bahmutov commented Apr 11, 2014

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

This comment has been minimized.

Show comment
Hide comment
@amio

amio Jun 10, 2014

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(/*...*/)

amio commented Jun 10, 2014

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

This comment has been minimized.

Show comment
Hide comment
@acrodrig

acrodrig Nov 18, 2014

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

acrodrig commented Nov 18, 2014

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

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Nov 18, 2014

Member

@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.

Member

mgol commented Nov 18, 2014

@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.

@mgol mgol reopened this Nov 18, 2014

@petebacondarwin petebacondarwin added this to the Backlog milestone Nov 18, 2014

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Nov 18, 2014

Member

Perhaps this is something that we can fix in 1.4?

Member

petebacondarwin commented Nov 18, 2014

Perhaps this is something that we can fix in 1.4?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Nov 25, 2014

Member

@petebacondarwin That would be great!

Member

mgol commented Nov 25, 2014

@petebacondarwin That would be great!

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Nov 25, 2014

Member

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

Member

mgol commented Nov 25, 2014

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

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Nov 25, 2014

Member

We have the "1.4 candidate" label at the moment

Member

petebacondarwin commented Nov 25, 2014

We have the "1.4 candidate" label at the moment

@bahmutov

This comment has been minimized.

Show comment
Hide comment
@bahmutov

bahmutov Dec 8, 2014

@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

bahmutov commented Dec 8, 2014

@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 btford self-assigned this Dec 11, 2014

@btford

This comment has been minimized.

Show comment
Hide comment
@btford

btford Dec 11, 2014

Contributor

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

Contributor

btford commented Dec 11, 2014

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

@petebacondarwin petebacondarwin modified the milestones: Backlog, 1.4.x Dec 15, 2014

@ocombe

This comment has been minimized.

Show comment
Hide comment
@ocombe

ocombe Dec 16, 2014

Contributor

@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).

Contributor

ocombe commented Dec 16, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Dec 16, 2014

Member

@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.

Member

pkozlowski-opensource commented Dec 16, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@ocombe

ocombe Dec 16, 2014

Contributor

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 !

Contributor

ocombe commented Dec 16, 2014

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

This comment has been minimized.

Show comment
Hide comment
@chrisadams

chrisadams Feb 3, 2015

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.

chrisadams commented Feb 3, 2015

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.

@QuentinFchx QuentinFchx referenced this issue Mar 3, 2015

Merged

Various Fixes #55

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource May 26, 2015

Member

Yeh, so this is really confusing, we should be throwing an exception in case of redefining a module, instead of overriding things silently. 1.5 might be a good candidate for it if it doesn't get solved in 1.4

Member

pkozlowski-opensource commented May 26, 2015

Yeh, so this is really confusing, we should be throwing an exception in case of redefining a module, instead of overriding things silently. 1.5 might be a good candidate for it if it doesn't get solved in 1.4

@nevcos

This comment has been minimized.

Show comment
Hide comment
@nevcos

nevcos Nov 5, 2015

+1 ... it's super error prone and super frustrating when shit happens.

A single flag would avoid wasting so much time in the world 😞 :

angular.module('foo', []).directive('bar', ...);
var overrideIfAlreadyCreated = true
angular.module('foo', [], overrideIfAlreadyCreated).filter('baz', ...);

nevcos commented Nov 5, 2015

+1 ... it's super error prone and super frustrating when shit happens.

A single flag would avoid wasting so much time in the world 😞 :

angular.module('foo', []).directive('bar', ...);
var overrideIfAlreadyCreated = true
angular.module('foo', [], overrideIfAlreadyCreated).filter('baz', ...);

@petebacondarwin petebacondarwin modified the milestones: 1.5.x - migration-facilitation, 1.4.x Nov 6, 2015

@Narretz Narretz modified the milestones: 1.5.x, 1.7.0, Backlog Apr 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment