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

Conversation

devversion
Copy link
Member

  • Sometimes the ngModel of the custom input element (provided by the user) does not have any initial $viewValue.
  • The chips component always expects the chipBuffer to be a string and a non-string reference can cause Runtime Exceptions.

Fixes #9867.

* Sometimes the ngModel of the custom input element (provided by the user) does not have any initial $viewValue.

* The chips component always expects the chipBuffer to be a string and a non-string reference can cause Runtime Exceptions.

Fixes angular#9867.
@devversion devversion added the needs: review This PR is waiting on review from the team label Oct 21, 2016
this.userInputElement[0].value;

// Ensure that the chip buffer is always a string. For example, the input element buffer might be falsy.
return angular.isString(chipBuffer) ? chipBuffer : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ever possible for chipBuffer to be something like 3 instead of the string '3'? If so, this may fail?

Copy link
Member Author

@devversion devversion Oct 24, 2016

Choose a reason for hiding this comment

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

Yeah, it could happen if the developer uses a custom input element or for example the md-autocomplete (and does have some custom ng-models)

This is kind of a safety check to ensure that there will be not Runtime errors ever.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed; I'm just wondering if we could alter this to cast the input to a string instead of returning an empty string.

So, perhaps something like:

return angular.isDefined(chipBuffer) ? chipBuffer + '' : ''

This will ensure that it is always a string, or the empty string, but should handle more cases. Thoughts?

Copy link
Member Author

@devversion devversion Oct 24, 2016

Choose a reason for hiding this comment

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

I thought a lot about this, and figured that just returning an empty string is the safest & cleanest approach IMO

I think casting would open more questions than it would solve, because then users might get weird chips like [Object], rather than not showing any chip.

I used the isString to avoid objects being used as truthy chip buffers.

Copy link
Contributor

@topherfangio topherfangio left a comment

Choose a reason for hiding this comment

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

LGTM 👍

return !this.userInputElement ? this.chipBuffer :
this.userInputNgModelCtrl ? this.userInputNgModelCtrl.$viewValue :
this.userInputElement[0].value;
var chipBuffer = !this.userInputElement ? this.chipBuffer :
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this code was here from before, but it might be a good chance to get rid of the nested ternary expression since it's a little hard to read/follow. Also the this.userInputElement[0].value can be reduced to this.userInputElement.val().

@ThomasBurleson ThomasBurleson added needs: presubmit and removed needs: review This PR is waiting on review from the team labels Oct 27, 2016
@ThomasBurleson ThomasBurleson modified the milestones: 1.1.2, 1.1.3 Oct 27, 2016
@ppham27
Copy link
Contributor

ppham27 commented Oct 27, 2016

Should this JSDoc annotation be changed if this.chipBuffer is not necessarily a string?

See: https://github.com/DevVersion/material/blob/4c0c7f9cc0fa7f24a108b08d849cf905183e6451/src/components/chips/js/chipsController.js#L79

@topherfangio
Copy link
Contributor

@ppham27 I think that's okay the way it is. Although you can technically force it to be something other than a string, it should always be a string.

@devversion
Copy link
Member Author

@ppham27 Yeah this is fine. Also the chipBuffer variable is always a string, because it is from our internal ngModel and can't be overwritten with custom ngModel formatters by the user.

@kara kara added pr: merge ready This PR is ready for a caretaker to review and removed needs: presubmit labels Nov 16, 2016
@kara kara merged commit d774b76 into angular:master Nov 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants