fix(filters): always call splice() with 2 arguments or more

[gkalpak]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.

What is the current behavior? (You can also link to an open issue here)
#14467

What is the new behavior (if this is a feature change)?
Consistent behavior in ES5 and ES6 environments.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

• The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

Other information:
When calling .splice() without a 2nd argument (deleteCount), most browsers will splice to the end of the array. As it turns out, this is the behavior specified in ES2015. In ES5, the spec seems to imply that nothing should happen.

To avoid inconsistent behavior among browsers implementing different versions of the EcmaScript standart or when ES5 shims are included (e.g. https://github.com/es-shims/es5-shim/), this commit ensures that all calls to .splice() provide at least two arguments.

Fixes #14467

[gkalpak]

(From a quick grepping through the codebase, these seem to be the only places where we called splice with less than 2 arguments.)

[Narretz]

LGTM. We should lool into an eslint task for that once we switch.

[gkalpak]

Can we add such specific rules to eslint ?
/cc @mgol

[mgol]

There is no built-in rule for that AFAIK (see http://eslint.org/docs/rules/) but if this is a real difference and browsers differ in behavior then perhaps we could get one landed in ESLint core, similar to the parseInt one. Otherwise we could write our own ESLint plugin for that.

I'd like to see which browsers behave in which way, though, before doing anything about that.

[gkalpak]

AFAIK, all browsers do it like ES6 (I didn't test with IE8, but that's not supported anyway).
The problem is when people include es5-shim and es5-shim decides it needs to patch the browser with the ES5 version of splice. I haven't tested, but Safari 7/8 are the only browsers I know are patched by es5-shim (could be others though).

It might not be worth it putting too much effort in it, since it will only affect a very small minority (and people are probably going to move to es6-shim soon enough).

[mgol]

If no browsers follow ES5 behavior here then es5-shim should stop patching that in considering what ES6 says. I'd rather get it fixed there rather than us working around bad shims.

[Narretz]

Browsers follow ES2015 or rather they have implemented the common sense
approach that was then written into ES2015. ES5 shim follows the es5 spec
explicitly.
Am 22.04.2016 14:43 schrieb "Michał Gołębiowski" notifications@github.com:

If no browsers follow ES5 behavior here then es5-shim should stop patching
that in considering what ES6 says. I'd rather get it fixed there rather
than us working around bad shims.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#14489 (comment)

[mgol]

IMO es5-shim shouldn't implement things in a way invalidated by future
versions of the spec, especially if no browsers followed the previous way.

Michał Gołębiowski