Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(loader): add convenience method for creating components #12933

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 112 additions & 0 deletions src/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,118 @@ function setupModuleLoader(window) {
*/
directive: invokeLaterAndSetModuleName('$compileProvider', 'directive'),

/**
* @ngdoc method
* @name angular.Module#component
* @module ng
* @param {string} name Name of the component in camel-case (i.e. myComp which will match as my-comp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@param {string} name name of the component in canonical camelCase form (i.e. ...

* @param {Object} options Component definition object (a simplified
* {@link ng.$compile#directive-definition-object directive definition object}),
* has the following properties (all optional):
*
* - `controller` – `{(string|function()=}` – Controller constructor function that should be
* associated with newly created scope or the name of a {@link ng.$compile#-controller-
* registered controller} if passed as a string. Empty function by default.
* - `controllerAs` – `{string=}` – An identifier name for a reference to the controller.
* If present, the controller will be published to scope under the `controllerAs` name.
* If not present, this will default to be the same as the component name.
* - `template` – `{string=|function()=}` – html template as a string or a function that
* returns an html template as a string which should be used as the contents of this component.
* Empty string by default.
*
* If `template` is a function, then it is {@link auto.$injector#invoke injected} with
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we deviating from the "normal" DDO's template/templateUrl (which don't get injected) ?
There's probably a good reason; I just want to know it :)

Maybe we should stress it more, because more experienced users might get biten by this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that in module.component there is no factory function. In classic DDO, the template and templateUrl properties are in the closure of the factory function and so have access to injectables from there.

* the following locals:
*
* - `$element` - Current element
* - `$attrs` - Current attributes object for the element
*
* - `templateUrl` – `{string=|function()=}` – path or function that returns a path to an html
* template that should be used as the contents of this component.
*
* If `templateUrl` is a function, then it is {@link auto.$injector#invoke injected} with
* the following locals:
*
* - `$element` - Current element
* - `$attrs` - Current attributes object for the element
* - `bindings` – `{object=}` – Define DOM attribute binding to component properties.
* Component properties are always bound to the component controller and not to the scope.
* - `transclude` – `{boolean=}` – Whether {@link $compile#transclusion transclusion} is enabled.
* Enabled by default.
* - `isolate` – `{boolean=}` – Whether the new scope is isolated. Isolated by default.
* - `restrict` - `{string=}` - String of subset of {@link ng.$compile#-restrict- EACM} which
* restricts the component to specific directive declaration style. If omitted, this defaults to 'E'.
* - `$canActivate` – `{function()=}` – TBD.
* - `$routeConfig` – `{object=}` – TBD.
*
* @description
* Register a component definition with the compiler. This is short for registering a specific
* subset of directives which represents actual UI components in your application. Component
* definitions are very simple and do not require the complexity behind defining directives.
* Component definitions usually consist only of the template and the controller backing it.
* In order to make the definition easier, components enforce best practices like controllerAs
* and default behaviors like scope isolation, restrict to elements and allow transclusion.
*
* Here are a few examples of how you would usually define components:
*
* ```js
* angular.module(...).component.('myComp', {
* template: '<div>My name is {{myComp.name}}</div>',
* controller: function MyCtrl() {
* this.name = 'shahar';
* }
* });
*
* angular.module(...).component.('myComp', {
* template: '<div>My name is {{myComp.name}}</div>',
* bindings: {name: '@'}
* });
*
* angular.module(...).component.('myComp', {
* templateUrl: 'views/my-comp.html',
* controller: 'MyCtrl as ctrl',
* bindings: {name: '@'}
* });
*
* ```
*
* See {@link ng.$compileProvider#directive $compileProvider.directive()}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be more explicit about the reasons why one would use this helper.
We should provide some example usage, including common cases where we rely upon the defaults.

*/
component: function(name, options) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be using invokeLaterAndSetModuleName?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just a wrapper around module.directive. module.directive will do the invokeLaterAndSetModuleName

function factory($injector) {
function makeInjectable(fn) {
if (angular.isFunction(fn)) {
return function(tElement, tAttrs) {
return $injector.invoke(fn, this, {$element: tElement, $attrs: tAttrs});
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this additional invoke via the $injector is an unwanted performance hit, since it would be called often, compared to the original factory function. But I guess that we can't get away from that given this API.

One alternative, could be to have a dependencies property on the options, which specifies all the dependencies that should be available in some closure to all these methods... Then we can inject these dependencies only once for all instances of the component, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how that would work, the methods already have a closure when they are placed on the options obj, I don't think we can update that closure. Best we could do is pass the dependencies as a map argument to the methods, but I think it really isn't worth it, it doesn't seem like a big performance hit for me.

Another option is to have module.component accept either a options object or a factory method and then if you want templateUrl/template function that use DI, you have to pass a factory instead of options. But again, I think having templateUrl/template functions injectable is much more elegant and not such a big perf hit. It is also consistent with how the controller and $canActivate will be invoked.

} else {
return fn;
}
}

var template = (!options.template && !options.templateUrl ? '' : options.template);
return {
controller: options.controller || function() {},
controllerAs: identifierForController(options.controller) || options.controllerAs || name,
template: makeInjectable(template),
templateUrl: makeInjectable(options.templateUrl),
transclude: options.transclude === undefined ? true : options.transclude,
scope: options.isolate === false ? true : {},
bindToController: options.bindings || {},
restrict: options.restrict || 'E'
};
}

if (options.$canActivate) {
factory.$canActivate = options.$canActivate;
}
if (options.$routeConfig) {
factory.$routeConfig = options.$routeConfig;
}
factory.$inject = ['$injector'];

return moduleInstance.directive(name, factory);
},

/**
* @ngdoc method
* @name angular.Module#config
Expand Down
111 changes: 111 additions & 0 deletions test/loaderSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,114 @@ describe('module loader', function() {
expect(window.angular.$$minErr).toEqual(jasmine.any(Function));
});
});


describe('component', function() {
it('should return the module', function() {
var myModule = window.angular.module('my', []);
expect(myModule.component('myComponent', {})).toBe(myModule);
});

it('should register a directive', function() {
var myModule = window.angular.module('my', []).component('myComponent', {});
expect(myModule._invokeQueue).toEqual(
[['$compileProvider', 'directive', ['myComponent', jasmine.any(Function)]]]);
});

it('should add router annotations to directive factory', function() {
var myModule = window.angular.module('my', []).component('myComponent', {
$canActivate: 'canActivate',
$routeConfig: 'routeConfig'
});
expect(myModule._invokeQueue.pop().pop()[1]).toEqual(jasmine.objectContaining({
$canActivate: 'canActivate',
$routeConfig: 'routeConfig'
}));
});

it('should return ddo with reasonable defaults', function() {
window.angular.module('my', []).component('myComponent', {});
module('my');
inject(function(myComponentDirective) {
expect(myComponentDirective[0]).toEqual(jasmine.objectContaining({
controller: jasmine.any(Function),
controllerAs: 'myComponent',
template: '',
templateUrl: undefined,
transclude: true,
scope: {},
bindToController: {},
restrict: 'E'
}));
});
});

it('should return ddo with assigned options', function() {
function myCtrl() {}
window.angular.module('my', []).component('myComponent', {
controller: myCtrl,
controllerAs: 'ctrl',
template: 'abc',
templateUrl: 'def.html',
transclude: false,
isolate: false,
bindings: {abc: '='},
restrict: 'EA'
});
module('my');
inject(function(myComponentDirective) {
expect(myComponentDirective[0]).toEqual(jasmine.objectContaining({
controller: myCtrl,
controllerAs: 'ctrl',
template: 'abc',
templateUrl: 'def.html',
transclude: false,
scope: true,
bindToController: {abc: '='},
restrict: 'EA'
}));
});
});

it('should allow passing injectable functions as template/templateUrl', function() {
var log = '';
window.angular.module('my', []).component('myComponent', {
template: function($element, $attrs, myValue) {
log += 'template,' + $element + ',' + $attrs + ',' + myValue + '\n';
},
templateUrl: function($element, $attrs, myValue) {
log += 'templateUrl,' + $element + ',' + $attrs + ',' + myValue + '\n';
}
}).value('myValue', 'blah');
module('my');
inject(function(myComponentDirective) {
myComponentDirective[0].template('a', 'b');
myComponentDirective[0].templateUrl('c', 'd');
expect(log).toEqual('template,a,b,blah\ntemplateUrl,c,d,blah\n');
});
});

it('should allow passing transclude as object', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want it to pass transclude as object ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One (long) word: multi-slot-transclusion (see a4ada8b)

window.angular.module('my', []).component('myComponent', {
transclude: {}
});
module('my');
inject(function(myComponentDirective) {
expect(myComponentDirective[0]).toEqual(jasmine.objectContaining({
transclude: {}
}));
});
});

it('should give ctrl as syntax priority over controllerAs', function() {
window.angular.module('my', []).component('myComponent', {
controller: 'MyCtrl as vm'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be even better to add controllerAs as well, to test that even that has lower priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is a realistic use case that the developer will pass string with ' as ' in controller and also controllerAs property

});
module('my');
inject(function(myComponentDirective) {
expect(myComponentDirective[0]).toEqual(jasmine.objectContaining({
controllerAs: 'vm'
}));
});
});
});