Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.8k
Improve Typescript definition for Map#1841
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
e5ee08e
to0d49ee8
CompareWorking today on seeing if |
mbullington commentedJul 16, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@jdeniau I spent a lot of time on Mind if I have write access to your fork so I can push my changes? |
Happy to work more on this with you as well, if you have a good idea of what else needs implementing in order to review & merge. |
@mbullington I gave you access on my fork. You can push on thejd-feat-tsMapObject branch. If you want to continue on the subject, there are still some issues with constructor like |
… into jd-feat-tsMapObject
mbullington commentedJul 19, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Was able to push my changes. Thinking about What's the issue with |
jdeniau commentedJul 20, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Nice implementation ! One think I'm not sure about (and something I did added) is the About |
@bdurrer I will open a discussion or a wiki page once this is merged to keep the CHANGELOG light |
mbullington commentedNov 5, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks all and@jdeniau for working on this! I can understand the argument that I think the main goal of this is to enable strong typing for the "immutable POJOs," which in a lot of cases due to Right now it's hard to justify using the Immutable typing with For people like@bdurrer 's use case I'd hate to flip this around, trading one alienated use case for another. If we can strike a balance (which I hope we've done?) both groups will be able to work productively. I've been playing around with an internal version for Mapbox Studio that recursively types I'd love some feedback on if these should be added or if the from/toJS logic is even correct: https://gist.github.com/mbullington/631dd21f910e594b29fb97dd6f3e62e8 |
4674d2d
tod627ad3
Comparetype-definitions/immutable.d.ts Outdated
// https://github.com/immutable-js/immutable-js/issues/1462#issuecomment-584123268 | ||
/** @ignore */ | ||
type GetMapType<S> = S extends MapFromObject<infer T> ? T : S; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This implies thatgetIn
will be used on something that isMapFromObject
all the way down. What happens when it includes a typicalMap
orList
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think that it will just return the typeMap
orList
, am I correct?@jdeniau
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
On something that is not aMapFromObject
, it will return the type directly. I do not really know in the end what will happen though. Maybe@mbullington who coded this part ?
I thing that:
MapFromObject<{m:Map<string,number>;l:List<number>;}>GetMapType<m['m']>// Map<string, number>GetMapType<m['l']>// List<number>
Reading this particular line, the function only extract the initial object of a MapFromObject, that's all.
So in a chain:
MapFromObject<{m:Map<string,List<number>>}>
CallingRetrievePath<R, ['m', 'some-string', number]>
should returnnumber
getIn<P extends ReadonlyArray<string | number | symbol>>( | ||
searchKeyPath: [...P], | ||
notSetValue?: unknown | ||
): RetrievePath<R, P>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Part of what has made typinggetIn
correctly challenging is that we don't necessarily know the nested types within. The recursive type you've defined here gets way closer, but if it's correct it needs to handle all possible cases thatgetIn
operates on in the nested layers. Then there's no reason it can't be used in allgetIn
methods
get<NSV>(key: string, notSetValue: NSV): NSV; | ||
// https://github.com/microsoft/TypeScript/pull/39094 | ||
getIn<P extends ReadonlyArray<string | number | symbol>>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A nested structure may have keys that are notstring | number | symbol
I think this needs to beReadonlyArray<any>
to account for all possible key types of a typicalMap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I do not know why, but if I do that, callinggetIn(['a', 'b', 'c', 'd'])
is not handled and fallback to be anever
type
notSetValue?: unknown | ||
): RetrievePath<R, P>; | ||
set<K extends keyof R>(key: K, value: R[K]): this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
similar toget
above, I think this needs another definition which covers wherekey
is not inkeyof R
with an appropriate return type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ifkey
is not inkeyof R
immutable will return aundefined
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@EduardoAraujoB if key is not inkeyof R
, then technically a new Map is returned:
Map({}).set('a','a');// output Map({ a: 'a' })
@leebyron Assaid on slack I prefered the solution of replicating TypeScript objects: you need to define the interface before:
consto={a:'a'};o.b='b';// Property 'b' does not exist on type '{ a: string; }'.
type-definitions/immutable.d.ts Outdated
* that does not guarantee the key was not found. | ||
*/ | ||
get<K extends keyof R>(key: K, notSetValue?: unknown): R[K]; | ||
get<NSV>(key: string, notSetValue: NSV): NSV; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should this bekey: any
here?K
alone can bestring | number | symbol
, but beyond I think we would expect that for any requested key that doesn't exist, we ask a NSV to be provided
// @ts-expect-error -- Not supported by typescript since 4.0.0 https://github.com/immutable-js/immutable-js/pull/1626 | ||
expect(m.get('a')).toBe('A'); | ||
// @ts-expect-error -- Not supported by typescript since 4.0.0 https://github.com/immutable-js/immutable-js/pull/1626 | ||
expect(m.get('b')).toBe('B'); | ||
// @ts-expect-error -- Not supported by typescript since 4.0.0 https://github.com/immutable-js/immutable-js/pull/1626 | ||
expect(m.get('c')).toBe('C'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm confused - is this not currently failing? Why does this PR cause this to start producing errors?
type-definitions/ts-tests/map.ts Outdated
// $ExpectError | ||
Map({ a: 4 }).get('b'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you add.get('b', undefined)
to test the alternate NSV definition?
Uh oh!
There was an error while loading.Please reload this page.
Map({ a: 4, b: true }).getIn(['a']); | ||
// $ExpectType number | ||
Map({ a: Map({ b: Map({ c: Map({ d: 4 }) }) }) }).getIn(['a', 'b', 'c', 'd']); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you add getIn tests where nested Map are constructed by other means (eg not onlyObjectFromMap
type?) as well as havingList
within?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unfortunatly, it's only working forMapFromObject
for now.
@@ -80,6 +115,28 @@ import { Map, List } from 'immutable'; | |||
// $ExpectType Map<number, string | number> | |||
Map<number, number | string>().set(0, 'a'); | |||
// $ExpectError | |||
Map({ a: 1 }).set('b', 'b'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm surprised by this, I'd expect eitherMapFromObject<{ a: 1, b: 'b' }>
orMap<string, number | string>
as a result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See comment above about the consistency with plain objects in TS
mbullington commentedDec 1, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I took a stab at making sure @leebyron Thanks for looking and identifying these issues with In honesty I changing My reasoning is we're dealing with both string literal types like This could present issues with typing I wonder if there's a better way of typing |
eesaber commentedApr 17, 2022
You can get the type of the given key path of a plain JS object like typeArr=readonlyunknown[];typeGetInType<T,KeyPathextendsArr>=KeyPathextends[inferU] ?UextendskeyofT ?T[U] :'fail 1' :KeyPathextends[inferK, ...inferRest] ?KextendskeyofT ?GetInType<T[K],Rest> :'fail 2' :'fail 3'// ---constobj={foo:{list:[1,2,3],listObj:[{name:"alex"},{name:"sam"}],},num:123,};typetest=GetInType<typeofobj,['foo','list']>; So, the type of For ImmutableJS, I guess we could replace |
faee487
tobeeac64
Comparejbaca303 commentedMar 14, 2023
What's the status on this? Looks like it would be highly beneficial for a project my team is working on |
@jbaca303 it's a work in progress but it's really complex to make this work properly. Don't expect it to be done in the next few weeks, but if you would like to try it on your project, every return would be appreciable. |
jbaca303 commentedMar 15, 2023
Thanks for the quick update. Totally understand that this complex. Thank you guys for working on this, really appreciate it! |
I am using this for nearly 2 years now athttps://github.com/mapado internally. I think it is time to go public now. I will release a RC on a 5.0 version to have some community return on this. Thanks you all for your patience. |
Uh oh!
There was an error while loading.Please reload this page.
Imagine the following code:
This was previously typed as
Map<string, string | number>
and return type of
m.get('length')
orm.get('inexistant')
was typed asstring | number | undefined
.This made
Map
really unusable with TypeScript.Now the Map is typed like this:
and the return type of
m.get('length')
is typed asnumber
.The return of
m.get('inexistant')
throw the TypeScript error:If you want to keep the old definition
This is a minor BC for TS users, so if you want to keep the old definition, you can declare you Map like this:
If you need to type the Map with a larger definition
You might want to declare a wider definition, you can type your Map like this:
Are all
Map
methods implemented ?For now, only
get
,set
andgetIn
methods are implemented. All other methods will fallback to the basicMap
definition. Other method definition will be added later, but as some might be really complex, we prefer the progressive enhancement on the most used functions.