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

test($compile): add test for optional require in directives with ^ operator #9392

Closed
wants to merge 2 commits into from
Closed

test($compile): add test for optional require in directives with ^ operator #9392

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4915363
test($compile): add test for optional `require` in directives with `^…
Oct 2, 2014
fe45f49
fix($compile): returning null when an optional controller is not found
Oct 2, 2014
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
36 changes: 36 additions & 0 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4095,6 +4095,42 @@ describe('$compile', function() {
});


it("should not throw an error if required controller can't be found and is optional",function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

module(function() {
directive('dep', function(log) {
return {
require: '?^main',
link: function(scope, element, attrs, controller) {
log('dep:' + !!controller);
}
};
});
});
inject(function(log, $compile, $rootScope) {
$compile('<div main><div dep></div></div>')($rootScope);
expect(log).toEqual('dep:false');
});
});


it("should not throw an error if required controller can't be found and is optional with the question mark on the right",function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't say "should not throw", say "should inject null" instead, I guess basically the controller should be null, not any other value

Copy link
Author

Choose a reason for hiding this comment

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

Looks like it's injecting "undefined" instead (or rather, not passing that arg at all). How about:
"should not pass a controller if required controller can't be found and is optional with the question mark on the right"

Copy link
Contributor

Choose a reason for hiding this comment

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

It's actually supposed to pass null --- but I see that it doesn't if the controller isn't found and it's optional.

That's a bug, you should fix it in this PR tbh, it will let us write better tests

Copy link
Author

Choose a reason for hiding this comment

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

Ok, so the actual behavior should be that it passes null if it's not found and optional. So it should say:
"should pass null if required controller can't be found and is optional with the question mark on the right"

I'll try to find time to fix the bug as part of this PR. Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

@caitp, I'm planning on adding || null to this line which fixes this bug.

                                            //  all tests pass when I add this ↓
value = value || $searchElement[retrievalMethod]('$' + require + 'Controller') || null;

Preparing a PR now. Let me know if you think that there's a better solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's perfect, should be fine

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually no --- I would just change the return value; to return value || null; --- much easier

Copy link
Author

Choose a reason for hiding this comment

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

That's what I went with initially, but then saw this line and thought it might make more sense to keep the value assignment in one place. But I personally like adding it to the return statement instead. I'll go with that.

module(function() {
directive('dep', function(log) {
return {
require: '^?main',
link: function(scope, element, attrs, controller) {
log('dep:' + !!controller);
}
};
});
});
inject(function(log, $compile, $rootScope) {
$compile('<div main><div dep></div></div>')($rootScope);
expect(log).toEqual('dep:false');
});
});


it('should have optional controller on current element', function() {
module(function() {
directive('dep', function(log) {
Expand Down