-
Notifications
You must be signed in to change notification settings - Fork 27.5k
feat(ngAsDirective): new as directive to publish component controllers into current scope #14080
Conversation
test/ng/directive/asSpec.js
Outdated
expect(scope.$ctrl.undamaged).toBe(true); | ||
expect($ctrl.myComponent).not.toBeUndefined(); | ||
expect($ctrl.myComponent && $ctrl.myComponent.property).toBe(expected); | ||
scope.$destroy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to destroy the scope (or dealoc the element, I believe).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ops! Right! Each test have a new shiny $rootScope, so not even new required.
About dealoc a I do not have clear if I must add or remove it, I found the following:
- tests: Use dealoc(element) for elements created with $compile() material#2250
angular.js/test/ng/compileSpec.js
Lines 738 to 748 in 1061c56
dealoc($compile('<span default-dir ></span>')($rootScope)); expect(log).toEqual('defaultDir'); log.reset(); dealoc($compile('<default-dir></default-dir>')($rootScope)); expect(log).toEqual('defaultDir'); log.reset(); dealoc($compile('<span class="default-dir"></span>')($rootScope)); expect(log).toEqual(''); log.reset();
And tests from:
angular.js/test/ng/compileSpec.js
Lines 10001 to 10015 in 1061c56
it('should register a directive', function() { angular.module('my', []).component('myComponent', { template: '<div>SUCCESS</div>', controller: function(log) { log('OK'); } }); module('my'); inject(function($compile, $rootScope, log) { element = $compile('<my-component></my-component>')($rootScope); expect(element.find('div').text()).toEqual('SUCCESS'); expect(log).toEqual('OK'); }); });
are using the global "element" variable cleaned by:angular.js/test/ng/compileSpec.js
Line 145 in 1061c56
dealoc(element);
But I'm not sure if the main afterEach is executed after the whole describe, or after each piece. Or may be it is a bug.
But if you are confident that it is not necessary I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm not confident, that's why I wrote "(or dealoc the element, I believe)". TBH, when dealoc
is needed and when it isn't is still a big mystery for me :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some investigation it seems that testabilityPatch
applies an afterEach
to all tests, that automatically cleans up $rootElement
and $rootScope
. This means that you don't have to manually call dealoc
if at least one of the following conditions holds:
- Any elements you create are in the DOM tree of the
$rootElement
(e.g. you call$rootElement.append(yourCreatedElement)
). - No other scope than
$rootScope
is sued for linking.
E.g. in this test, if you keep using $rootScope.$new()
, then you need to dealoc
(either the element or the scope) - yup, scopes can be dealoc
'd too 😃
If you switch to using $rootScope
, then I think you don't need dealoc
ing.
The good thing is that if you don't clean up properly, you'll get some warnings in the console, so you know something's wrong.
This is interesting, but still needs some work imo. A few thoughts:
|
Thanks for aaaaalllll the feedback! About thoughts:
Thanks a lot for your feedback! 😄 |
Although I really quite like the idea, and I can see a number of potentially nice uses for it, such as comparison validators, such as password confirmation, I am really nervous about how easy it would be to cause memory leaks, if one of these references was held on to after the DOM element containing the original value was destroyed. I think that Angular 2 can get away with this because its vars are only available to the containing template. Is that right or do they have additional mechanisms in place? |
NullificationI have already applied the nullification of the controller saved value when its component is destroyed (it overwrites the previously published controller object by null). So it does not matter if any parent scope holds the object where it is assigned, it is nullified. Beyond that point, I believe that if it is misused it should be a programmers fault. From my point of view, it is like using An excerpt from the test (notice that it crosses ng-if scope): it('should nullify the variable once the component is destroyed', function() {
$rootScope.$ctrl = {};
$compile('<div ng-if="!nullified"><my-component as="$ctrl.myComponent"></my-component></div>')($rootScope);
$rootScope.$apply('nullified = false');
expect($rootScope.$ctrl.myComponent).toBe(myComponentController);
$rootScope.$apply('nullified = true');
expect($rootScope.$ctrl.myComponent).toBe(null);
}); Anyway it is a good idea to think a little bit more about possible memory leaks.
|
+1 Good work! |
OK I think we are going the right direction here... Let me review properly later this week |
As a slight variation on this, what if we were to allow an expression rather than a simple property binding? I find that a registration/de-registration pattern is very common in components that I wrote in order to work around ngRepeat/ngIf/other asynchronous behaviors where child directives come in at a later date As a result, having this be an expression could simplify those use cases while still allowing this one: as="myToggle = $child" However a registration function could also be invoked as="toggleDidArrive($child)" The latter would not be possible without watching the property. When we figure out the logic for the |
I don't see why this will be an issue with the Do I miss something ? |
A small clarification, when I said "Any of them can be added in the future without breaking changes", I also mean that adding them now is like open the box of pandora, go back is a breaking change. So, if it is unclear how to proceed, I can go for the basic one with illegal variable name checking (if not checked, adding new features would be breaking changes). But to make things easier: <audio ng-ref="$ctrl.audioPlayer">
<source src="pop.mp3" type="audio/mpeg">
</audio> <toggle ng-ref="toggle">...</toggle>
<div ng-init="$ctrl.toggle = toggle"></div> function MyCompCtrl($scope) {
this.proceed = function() {
$scope.toggle.close();
};
} |
I have added three different commits, each one for one part of the implementation. |
Joining the discussion ... regarding what can be referenced or what not in response to #14080 (comment)
This seems to work fine, but is it good enough? What happens when you have multiple components of the same kind, but without ng-repeat (which means you cannot use index)? Should there be an argument to ngRef that will always create an array of references? |
My thoughts:
|
My thoughts on your thoughts :)
|
Close to landing as #16511 |
…o scope Thanks to @drpicox for the original implementation: PR angular#14080
…o scope Thanks to @drpicox for the original implementation: PR angular#14080
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) Feature.
What is the current behavior? (You can also link to an open issue here):
With restrict you can reference parent controllers from a children, but if you want to reference children components you need to find them with jqLike and then get the controller.
What is the new behavior (if this is a feature change)?:
It adds a new directive that assigns the controller of the current DOM element component to any expression. If the element is destroyed (ex: it was inside ngIf or ngRepeat) it assigns a null.
Does this PR introduce a breaking change?
No.
Please check if the PR fulfills these requirements
Other information:
I present in this PR the implementation of a directive that I use in almost all my projects (and other projects in which I'm consultant) since the last year.
Basic use
It is really simple: it publishes the controller of component in the current scope. Ex:
so it can be accessed from enclosing components.
Until this point, it is a kind of custom
controllerAs
property for parent scope.Using with parent components
In addition, as following the recommendation did by @petebacondarwin in #10007 (comment) it is able to assign it to any value, event enclosing controller. Ex:
Keep track of DOM elementsIt also serves to look for exact DOM elements instead of relaying in jqLite searches. In case that there is no a controller, the as directive assigns the current DOM element to the expression set.
Deprecating ngController
Why?
ngController is great for SEO webpages, but it has the inconvenience that it does not integrates well with bindings and many things have to be done "manually". For example:
In this example it needs to access to
$attrs
manually to find the bookId. You can imagine more complex functionalities, but I think that this is just enough to expose.Now, imagine that we do a directive to handle the same behaviour:
It has a conceptual problem: you must know by heart that static-book-detail publishes the
bookDetailCtrl
.Alternatively you can use a component, which always use
$ctrl
like:but this naive solution does not works because
$ctrl
is not defined in the same scope that code relies.Popossal
Finally, using as you can have this solution that seems cleaner and more easy to understand:
Additional Notes:
ngAs
directive behaviour is very close to Angular2#
template variables. It also copies concepts from RiotJS and Polymer (at least first versions).