Skip to content

Added type guard overload ReadonlyArray.prototype.every #18372

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

Closed
wants to merge 1 commit into from

Conversation

NaridaL
Copy link
Contributor

@NaridaL NaridaL commented Sep 10, 2017

Fixes #14963

@NaridaL
Copy link
Contributor Author

NaridaL commented Sep 10, 2017

Do the test.symbols and the test.types files not get generated if there are errors? Is there a better way of testing for errors so that those still get generated?

@NaridaL NaridaL force-pushed the array-every-type-pred branch from f8887c1 to cadf438 Compare September 10, 2017 19:18
const numberReadonlyArray: ReadonlyArray<number> = baseReadonlyArray; // should be ReadonlyArray<number>
}

if (baseReadonlyArray.every((x): x is number => "number" === typeof x)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, something's not working with the inference...?

@RyanCavanaugh
Copy link
Member

Do the test.symbols and the test.types files not get generated if there are errors?

Correct. Best practice is to have one testcase with all the error cases, and one testcase with zero errors. This is also better for future reviews since a file going from 0 to 1 errors will raise a red flag more quickly than a file going from e.g. 4 to 5.

@NaridaL NaridaL force-pushed the array-every-type-pred branch from cadf438 to e5a85e7 Compare September 11, 2017 13:04
@NaridaL
Copy link
Contributor Author

NaridaL commented Sep 11, 2017

OK, in the current state, there should be no errors. For some reason it's not working if the type parameter isn't specified explicitly. I'm not sure what the issue is.

@NaridaL NaridaL force-pushed the array-every-type-pred branch from e5a85e7 to ffd2d13 Compare September 18, 2017 22:21
@NaridaL
Copy link
Contributor Author

NaridaL commented Sep 18, 2017

OK, the issue was #18562, I've used a workaround, the tests pass as they should. This PR should be mergeable as it is now.

@NaridaL NaridaL force-pushed the array-every-type-pred branch from ffd2d13 to fc29212 Compare September 19, 2017 07:50
@microsoft microsoft deleted a comment from msftclas Sep 27, 2017
@NaridaL
Copy link
Contributor Author

NaridaL commented Oct 2, 2017

@RyanCavanaugh any comment on the current state of the PR?

@RyanCavanaugh
Copy link
Member

@mhegazy 👍 ?

@mhegazy
Copy link
Contributor

mhegazy commented Oct 9, 2017

We should have a similar change in Array as well. Array and ReadonlyArray should stay in sync.

@NaridaL
Copy link
Contributor Author

NaridaL commented Oct 10, 2017

@mhegazy Does that make sense? according to the linked issue

declare const strNumArr: (string | number)[]
const strNumArr2: (string | number)[] = strNumArr
if (strNumArr.every(isString)) {
   strNumArr2.push(23)
   // strNumArr now has type string[] and contains a number
}

... although tbh you can get the same prob with a ReadonlyArray with only slightly more effort.

@NaridaL
Copy link
Contributor Author

NaridaL commented Jan 16, 2018

@mhegazy Considering my last comment, this should be mergeable as-is? Or do we want to revisit the original issue?

@shanehsu
Copy link

@NaridaL Hope this get merged soon. Relying on .filter now. 👍

@DanielRosenwasser
Copy link
Member

I think we're just waiting on having the appropriate overload in Array for consistency

@mhegazy
Copy link
Contributor

mhegazy commented May 1, 2018

Sorry for the late reply. i do not see why we should not do that for Array. we already handle filter the same way. Array and ReadonlyArray should always stay in sync.

@NaridaL
Copy link
Contributor Author

NaridaL commented May 1, 2018

@mhegazy

.filter() creates a new array. .every() doesn't so including the same overload for Array could lead to issues such as the one I describe here #18372 (comment). Does that not make sense? As far as I can tell this is the same conclusion reached in the base issue. #14963

@shanehsu
Copy link

shanehsu commented May 1, 2018

This is a language issue then. TypeScript compiler have to track every reference assignment. More static analysis-like thing. Out of our hands now I suppose.

@typescript-bot
Copy link
Collaborator

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

@simonbuchan
Copy link

@mhegazy I just hit this now, would you like an updated PR with the Array variant? Should I keep the current test location, or would it make sense to put it in tests/cases/conformance/experssions/typeGuards?

I noticed tests/cases/compiler/arrayFilter.ts doesn't test the type-guardiness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants