make splice deleteCount required in es5.d.ts

[tjenkinson]

In ES5 deleteCount is not an optional argument. If it is not provided it defaults to 0 as a side effect of undefined being converted to an integer.
https://www.ecma-international.org/ecma-262/5.1/#sec-15.4.4.12

In ES6 deleleteCount is optional, and it defaults to the length of the array minus the start index.
https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.splice

If you are targeting ES5 but don't provide deleteCount the behaviour will be different depending on the environment your build is running in.

This PR makes the deleteCount required when targeting ES5 but should allow it to be optional for ES6.

fixes #32638

The Issue

When running in ES5

const input = [1, 2, 3];
const out = in.splice(0);
console.log(input); // [1, 2, 3]
console.log(out); // []

When running in ES6

const input = [1, 2, 3];
const out = in.splice(0);
console.log(input); // []
console.log(out); // [1, 2, 3]

[msftclas]

All CLA requirements met.

[RyanCavanaugh]

@typescript-bot test this
@typescript-bot user test this

[typescript-bot]

Heya @RyanCavanaugh, I've started to run the parallelized community code test suite on this PR at 368d007. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Heya @RyanCavanaugh, I'm starting to run the extended test suite on this PR at 368d007. Hold tight - I'll update this comment with the log link once the build has been queued.

[typescript-bot]

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

[RyanCavanaugh]

@weswigham looks like some noise in the user suite diff - can you check in on it?

[weswigham]

I opened #32610 to fix the chrome devtools changes awhile ago, the other changes (to the azure sdk which is happily updating rush and such under us, and to zone.js which seems to be broken slightly (no ides if that's our fault)) just need to be accepted.

[tjenkinson]

Anything I can do to help here?

[tjenkinson]

@RyanCavanaugh @weswigham any update?

[tjenkinson]

I brought this back up to date with master

[sandersn]

Thanks for your patience on this!

[RyanCavanaugh]

This caused a break

const arr: string[] = [];
const boundSplice = arr.splice.bind(arr, 0, 0);
const items: string[] = [];
// Used to be OK, now an error
boundSplice.apply(arr, items);

[ljharb]

@RyanCavanaugh it's OK in ES6, it's not OK in ES5 (altho there may have been pre-ES6 browsers that considered it optional). is that a type error when targeting ES6?

[RyanCavanaugh]

It's incorrectly a type error right now against ES6

[tjenkinson]

Shame this is getting reverted. We had an outage on our TV app which this would have caught if it had existed. A few TV’s are still way back in the past.

[tjenkinson]

Do you know why it caused bind and apply to break?

[ljharb]

@RyanCavanaugh so does that mean this PR needed to also override it in ES6 to make it optional?

[DanielRosenwasser]

@tjenkinson I believe it's because under strictBindCallApply, we try to infer the types from the signature, but overloads don't play well with that functionality. So @ljharb, if you had ES6 as the target, that extra overload caused issues. We believe that the most reasonable solution is to just say it's optional.

[tjenkinson]

Ok thanks for the explanation.

It’s a shame but not the end of the world because es5-shim protects us now :). I guess we could also add the definition to our projects manually.

[ljharb]

@DanielRosenwasser it seems like fixing the underlying flaws with overloads is a pretty important thing to address :-/

[DanielRosenwasser]

I agree, but I don't think we have anything concrete that we feel good about yet.