Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for ReadonlyArray to Array.isArray #22942

Closed
wants to merge 2 commits into from

Conversation

Jessidhia
Copy link

@Jessidhia Jessidhia commented Mar 28, 2018

Fixes #17002

This makes the Array.isArray test assert that the argument is ReadonlyArray instead of Array. This actually works well as the compiler preserves the Array-ness if the input could have been a full Array instead of just a ReadonlyArray; as demonstrated by the test.

This is arguably a breaking change if the argument actually is any and the user was expecting to get a mutable array from the output; but blindly mutating some any just because it passes Array.isArray sounds like a terrible idea so I think it's a good break.

@Jessidhia Jessidhia changed the title Add support to ReadonlyArray to Array.isArray Add support for ReadonlyArray to Array.isArray Mar 28, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Mar 31, 2018

looks like we are trading one set of problems with another. we would rather not do change it.

@mhegazy mhegazy closed this Mar 31, 2018
@Jessidhia Jessidhia deleted the is-array-readonlyarray branch April 1, 2018 07:21
@Jessidhia
Copy link
Author

Unfortunately the status quo is that using Array.isArray() when your input is a ReadonlyArray results in a non-implicit any[], which is less safe.

Adding a local overload should work in the meantime, though.

declare global {
  interface ArrayConstructor {
    isArray(arg: any): arg is ReadonlyArray<any>;
  }
}

@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants