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

Commit

Permalink
feat($compile): optionally get controllers from ancestors only
Browse files Browse the repository at this point in the history
Implement option to strengthen require '^' operator, by adding another '^'.

When a second '^' is used, the controller will only search parent nodes for the
matching controller, and will throw or return null if not found, depending on
whether or not the requirement is optional.

Closes #4518
Closes #4540
Closes #8240
Closes #8511
  • Loading branch information
Caitlin Potter authored and caitp committed Sep 26, 2014
1 parent b9df121 commit 07e3abc
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 8 deletions.
32 changes: 24 additions & 8 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,11 @@
* * (no prefix) - Locate the required controller on the current element. Throw an error if not found.
* * `?` - Attempt to locate the required controller or pass `null` to the `link` fn if not found.
* * `^` - Locate the required controller by searching the element and its parents. Throw an error if not found.
* * `^^` - Locate the required controller by searching the element's parents. Throw an error if not found.
* * `?^` - Attempt to locate the required controller by searching the element and its parents or pass
* `null` to the `link` fn if not found.
* * `?^^` - Attempt to locate the required controller by searching the element's parents, or pass

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Oct 2, 2014

Member

Am I misunderstanding this? It appears that here it indicates ?^^ while in the tests it's ^^?

This comment has been minimized.

Copy link
@caitp

caitp Oct 2, 2014

Contributor

both work --- the regexp explicitly supports both

This comment has been minimized.

Copy link
@caitp

caitp Oct 2, 2014

Contributor

we might be missing a test case, please file a bug or submit a PR to add it =)

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Oct 2, 2014

Member

That sounds like an easy way for me to get involved (for the first time finally). Should be pretty much a copy/paste of the test with the ^^? right? I'll give it a look.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 2, 2014

Member

Fun fact: It will even accept ^^?^^, ^?^^, ^^?^, ^?^.
(Beware though, that in those cases only the ^ preceeding ? are taken into account.)

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Oct 2, 2014

Member

@gkalpak, fascinating. If your statement "only the ^ preceeding ? are taken into account" is true then why would ^?^ work?

@caitp, I think we'd be fine to just test ?^^ and ^^? (I just finished adding that and running all the tests, they passed)... But perhaps instead we could test the regex itself? Just a thought...

This comment has been minimized.

Copy link
@caitp

caitp Oct 2, 2014

Contributor

I would prefer to test functionality --- particularly, the functionality that we actually want (not the side effects mentioned by @gkalpak) --- this way if the regexp ever changes, the tests don't become outdated

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Oct 2, 2014

Member

👍, good point. However, in that case it may be wise to make the matching regex more strict so it only matches the cases that are tested. If people are relying on something that isn't tested (like they're using ?^^ as that's what is documented) then they have no protection that this wont change underneath them.

I suggest that we make the regex only match ?^^ and ^^?, then make sure both of those cases are tested (which they will be in my test case). I can include making the regex more strict in my PR if you like.

This comment has been minimized.

Copy link
@caitp

caitp Oct 2, 2014

Contributor

I prefer the option that is the least code --- side effects are fine, nobody has a good reason to use them

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Oct 2, 2014

Member

Sorry, not sure I know what you're suggesting. So are you saying we should just leave it as it is and not worry about adding a unit test to check for ?^^ as well as ^^?? I believe we should at least test the case that's documented. So either change the documentation to be ^^? or change the test to test ?^^. Or test both cases. What do you recommend? I can PR either.

This comment has been minimized.

Copy link
@caitp

caitp Oct 2, 2014

Contributor

no, I think we should add more tests --- I don't think we need to worry about the case where arrows appear on both sides, because nobody has a reason to use that --- if we can make the code smaller, than sure, but otherwise it's not worth it

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Oct 2, 2014

Member

Ok, great. So I'll PR what I have now. It tests ?^^. It's pretty much a copy/paste of the other test. I think that should be sufficient. Thanks @caitp.

* `null` to the `link` fn if not found.
*
*
* #### `controllerAs`
Expand Down Expand Up @@ -567,7 +570,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
Suffix = 'Directive',
COMMENT_DIRECTIVE_REGEXP = /^\s*directive\:\s*([\d\w_\-]+)\s+(.*)$/,
CLASS_DIRECTIVE_REGEXP = /(([\d\w_\-]+)(?:\:([^;]+))?;?)/,
ALL_OR_NOTHING_ATTRS = makeMap('ngSrc,ngSrcset,src,srcset');
ALL_OR_NOTHING_ATTRS = makeMap('ngSrc,ngSrcset,src,srcset'),
REQUIRE_PREFIX_REGEXP = /^(?:(\^\^?)?(\?)?(\^\^?)?)?/;

// Ref: http://developers.whatwg.org/webappapis.html#event-handler-idl-attributes
// The assumption is that future DOM event attribute names will begin with
Expand Down Expand Up @@ -1589,22 +1593,34 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

function getControllers(directiveName, require, $element, elementControllers) {
var value, retrievalMethod = 'data', optional = false;
var $searchElement = $element;
var match;
if (isString(require)) {
while((value = require.charAt(0)) == '^' || value == '?') {
require = require.substr(1);
if (value == '^') {
retrievalMethod = 'inheritedData';
}
optional = optional || value == '?';
match = require.match(REQUIRE_PREFIX_REGEXP);
require = require.substring(match[0].length);

if (match[3]) {
if (match[1]) match[3] = null;
else match[1] = match[3];
}
if (match[1] === '^') {
retrievalMethod = 'inheritedData';
} else if (match[1] === '^^') {
retrievalMethod = 'inheritedData';
$searchElement = $element.parent();
}
if (match[2] === '?') {
optional = true;
}

value = null;

if (elementControllers && retrievalMethod === 'data') {
if (value = elementControllers[require]) {
value = value.instance;
}
}
value = value || $element[retrievalMethod]('$' + require + 'Controller');
value = value || $searchElement[retrievalMethod]('$' + require + 'Controller');

if (!value && !optional) {
throw $compileMinErr('ctreq',
Expand Down
37 changes: 37 additions & 0 deletions test/ng/compileSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3672,6 +3672,43 @@ describe('$compile', function() {
});


it('should get required parent controller', function() {
module(function() {
directive('nested', function(log) {
return {
require: '^^?nested',

This comment has been minimized.

Copy link
@kentcdodds

kentcdodds Oct 2, 2014

Member

Here it's ^^? whereas above it's ?^^

controller: function($scope) {},
link: function(scope, element, attrs, controller) {
log(!!controller);
}
};
});
});
inject(function(log, $compile, $rootScope) {
element = $compile('<div nested><div nested></div></div>')($rootScope);
expect(log).toEqual('true; false');
});
});


it('should throw if required parent is not found', function() {
module(function() {
directive('nested', function() {
return {
require: '^^nested',
controller: function($scope) {},
link: function(scope, element, attrs, controller) {}
};
});
});
inject(function($compile, $rootScope) {
expect(function() {
element = $compile('<div nested></div>')($rootScope);
}).toThrowMinErr('$compile', 'ctreq', "Controller 'nested', required by directive 'nested', can't be found!");
});
});


it('should get required controller via linkingFn (template)', function() {
module(function() {
directive('dirA', function() {
Expand Down

0 comments on commit 07e3abc

Please sign in to comment.