fix(linky): throw error if input is not a string

[gkalpak]

BREAKING CHANGE:

Before this change, the filter assumed that the input (if not undefined/null) was of type 'string'
and that certain methods (such as .match()) would be available on it. Passing a non-string value
would most likely result in a not-very-useful error being thrown (trying to call a method that does
not exist) or in unexpected behavior (if the input happened to have the assumed methods).

After this change, a proper (informative) error will be thrown. If you want to pass non-string
values through linky, you need to explicitly convert them to strings first.
Since input values could be initialized asynchronously, undefined or null will still be
returned unchanged (without throwing an error).

Closes #13547

[gkalpak]

TBH, I don't expect this change to actually break (m)any apps, since an error was thrown anyway (unless non-string inut had a "properly" behaving .match() method, which is unlikely).

We should, of course, keep the BC notice, but I wouldn't be too concerned about it.

[lgalfaso]

just two observations:

• looks inconsistent (eg not to throw on false but throw on true)
• due to the BC nature and that we are in the final stages for 1.5, I would push this to 1.6

[petebacondarwin]

I think this is not really much of a BC. It is really just providing a better error message for an invalid input.

Regarding the fact that it does not throw on false: this is standard practice for filters, since often they receive null or undefined initially while waiting for watch values to kick in.

[lgalfaso]

@petebacondarwin I am ok with doing nothing with null/undefined, but handling true and false different, sounds strange

[petebacondarwin]

Yes, I take it back in other filters we now test == null. E.g. https://github.com/angular/angular.js/blob/master/src/ng/filter/orderBy.js#L201
We should do this here too.

[gkalpak]

I tried to stay as close as possible to what happens now (in order to minimize the impact of the BC). Currently, falsy values are returned as is (i.e. won't throw an error).

But, I agree the current behavior is not correct. I'll make the suggested change and put this on the 1.6 milestone.

[gkalpak]

Updated, PTAL.

[petebacondarwin]

LGTM

[lgalfaso]

I am ok one way or another, but should we short-circuit this for the case that text === ''?

[petebacondarwin]

We might as well. It is in a separate module and so it doesn't impact on the size of the core angular.js file. It saves a few microseconds per digest if the string is empty as we don't need to run the regex or the sanitizer.

[lgalfaso]

One small comment that can be ignored. Otherwise, LGTM

[petebacondarwin]

Since it is barely a much of a breaking change I would like to get this into RC 1 - are there any strong objections to this?

[lgalfaso]

If @petebacondarwin is ok with this making it into 1.5, then I am ok

[petebacondarwin]

Let's add the short-circuit and merge into RC 1

[petebacondarwin]

Smashing!

[petebacondarwin]

@gkalpak - you need to put the "Closes" statements before the "BREAKING CHANGES" statement, because the changelog.js generator doesn't strip them out of the BREAKING CHANGES block.

[gkalpak]

Oops, you're right ! Noted ;)