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

kentcdodds · 2014-10-02T16:30:43Z

The directive property require allows optional requirement via
the ? before or after the ^ operator. Add tests to ensure this
functionality is not lost inadvertently.

Closes #9391

…` operator The directive property `require` allows optional requirement via the `?` before or after the `^` operator. Add tests to ensure this functionality is not lost inadvertently. Closes #9391

kentcdodds · 2014-10-02T16:31:08Z

And here are more tests for require: '?^main' and require: '^?main' @caitp. Thanks!

caitp · 2014-10-02T18:49:07Z

test/ng/compileSpec.js

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


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

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"

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

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!

@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.

That's perfect, should be fine

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

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.

Currently, `undefined` is returned. However, the desired behavior is to return `null` when the controller is optional and not found. Closes #9404

kentcdodds · 2014-10-02T22:44:31Z

There's the fix @caitp, thanks!

Currently, `undefined` is returned. However, the desired behavior is to return `null` when the controller is optional and not found. (If this breaks your app, it really shouldn't .v.) Closes #9404 Closes #9392

test($compile): add test for optional require in directives with `^…

4915363

…` operator The directive property `require` allows optional requirement via the `?` before or after the `^` operator. Add tests to ensure this functionality is not lost inadvertently. Closes #9391

mary-poppins added cla: yes and removed cla: no labels Oct 2, 2014

caitp reviewed Oct 2, 2014
View reviewed changes

jeffbcross force-pushed the master branch from cad9560 to f294244 Compare October 2, 2014 22:09

kentcdodds mentioned this pull request Oct 2, 2014

require on directives should pass null when the controller is not found and is optional #9404

Closed

fix($compile): returning null when an optional controller is not found

fe45f49

Currently, `undefined` is returned. However, the desired behavior is to return `null` when the controller is optional and not found. Closes #9404

btford force-pushed the master branch from 9011a65 to 46db47b Compare October 6, 2014 22:54

btford added this to the Backlog milestone Oct 7, 2014

jeffbcross force-pushed the master branch 4 times, most recently from e8dc429 to e83fab9 Compare October 10, 2014 17:38

petebacondarwin force-pushed the master branch from 4dd5a20 to 998c61c Compare October 19, 2014 10:58

caitp closed this in 4bf254c Oct 24, 2014

kentcdodds deleted the add-test-for-optional-required-controller branch October 24, 2014 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kentcdodds commented Oct 2, 2014

kentcdodds commented Oct 2, 2014

caitp Oct 2, 2014

kentcdodds Oct 2, 2014

caitp Oct 2, 2014

kentcdodds Oct 2, 2014

kentcdodds Oct 2, 2014

caitp Oct 2, 2014

caitp Oct 2, 2014

kentcdodds Oct 2, 2014

kentcdodds commented Oct 2, 2014

		});


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

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

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

Conversation

kentcdodds commented Oct 2, 2014

kentcdodds commented Oct 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Oct 2, 2014

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

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