Add separate types for Collection first()/last() with zero arguments #1653
Conversation
|
Glad to help :) Force pushed an updated commit! I'm not much of a flow user (and, frankly, quite the beginner in typescript as well), so the tests are passing, and "it looks good to me (tm)", but the flow part of this patch comes without warranty :) (Not quite sure why the travis build failed, it worked yesterday, and it works on my machine if I do |
|
Looks good to me! On the more fundamental stuff like this, flow and TS are basically identical. |
|
Cool! Not sure what to do about the failing tests. |
|
Found the cause of the build error, created a separate pull request #1656 for that. Will rebase this pull request against master whenever master is updated :) |
There was a problem hiding this comment.
So far the existing signature has worked since not providing the optional argument causes NSV to take the value undefined, which appropriately matches the type signature for get (get(key: K): V | undefined) since the collection could be empty.
Should this new type signature explicitly include | undefined like the type signature for get? Or are you proposing weakening the types for first() and last()? If so would you also propose weakening them consistently throughout other type signatures which could return undefined for empty collections?
type-definitions/Immutable.d.ts
Outdated
| @@ -3878,15 +3878,17 @@ declare module Immutable { | |||
| * In case the `Collection` is empty returns the optional default | |||
| * value if provided, if no default value is provided returns undefined. | |||
| */ | |||
| first<NSV>(notSetValue?: NSV): V | NSV; | |||
| first<NSV>(): V; | |||
There was a problem hiding this comment.
Should this be V | undefined?
When the type is `first<NSV>(notSetValue?: NSV): V | NSV`, the type of
the return value will always be `V | NSV` and you will have to handle
the possibility of the return value as being both of type V and
NSV. When you call `first` without specifying the type, it will default
to the type {}.
This patch changes the type of `first` without a type argument to be
that type or undefined, instead of that type or NSV, which defaults to
`{}` when not specified.
|
Very good point! That makes this patch much less important, I agree that the correct type is |
|
I just found a case where the explicit typing of Currently, the type of Temporary workaround is to use |
|
Fixed in #1676 ftr :) |
When the type is
first<NSV>(notSetValue?: NSV): V | NSV, the type ofthe return value will always be
V | NSVand you will have to handlethe possibility of the return value as being both of type V and
NSV. When you call first without specifying the type, it will default
to the type {}.
An example:
This patch will make the first succeed, by letting the type system
know that if no argument is passed to first(), the type will be String
in this case.