Skip to content
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

Potential bug in registerComponent #14391

Closed
rszachtsznajder opened this Issue Apr 7, 2016 · 0 comments

Comments

Projects
None yet
2 participants
@rszachtsznajder
Copy link

rszachtsznajder commented Apr 7, 2016

Description

Bug exists on angular 1.5.3 and even on master branch.

In compile.js at the beginning of registerComponent (angular.component) method there is a assignment:

var controller = options.controller || noop;

Some lines later (after factory function definition) the controller variable is modified by assigning component options starting with $ sign.

forEach(options, function(val, key) {
  if (key.charAt(0) === '$') {
    factory[key] = val;
    // Don't try to copy over annotations to named controller
    if (isFunction(controller)) controller[key] = val;
  }
});

This may modify an angular method noop if component doesn't have a controller, which should not have happened.

How I found it?

I came across this problem using ngComponentRouter 2.0.0 with Angular 1.5.3 when I had two components without controllers defined. In this situation when factory function is executed, the default controller (noop) has $routeConfig property from the later one defined component (because registerComponent modify the same object: noop function). Result: ngComponentRouter couldn't find properly defined route.

Fix proposal

Probably the fix is fairly simple. Use function() {} instead of noop:

var controller = options.controller || function() {};

This will guarantee new function object at any situation.

@gkalpak gkalpak added this to the 1.5.4 milestone Apr 8, 2016

gkalpak added a commit to gkalpak/angular.js that referenced this issue Apr 9, 2016

fix($compile): do not use `noop()` as controller for multiple components
Currently, custom annotations are copied from the CDO onto the controller constructor.
Using `noop()` when no controller has been specified, pollutes it with custom annotations and
makes one component's annotations available to all other components that have `noop()` as their
controller.

Fixes angular#14391

petebacondarwin added a commit that referenced this issue Apr 11, 2016

fix($compile): do not use `noop()` as controller for multiple components
Currently, custom annotations are copied from the CDO onto the controller constructor.
Using `noop()` when no controller has been specified, pollutes it with custom annotations and
makes one component's annotations available to all other components that have `noop()` as their
controller.

Fixes #14391
Closes #14402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.