Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
jdeniau merged 32 commits intoimmutable-js:5.xfromjdeniau:jd-feat-tsMapObject
Aug 24, 2023

Conversation

jdeniau
Copy link
Member

@jdeniaujdeniau commentedJul 6, 2021
edited
Loading

Imagine the following code:

constm=Map({length:3,1:'one'});

This was previously typed asMap<string, string | number>

and return type ofm.get('length') orm.get('inexistant') was typed asstring | number | undefined.

This madeMap really unusable with TypeScript.

Now the Map is typed like this:

MapOf<{length:number;1:string;}>

and the return type ofm.get('length') is typed asnumber.

The return ofm.get('inexistant') throw the TypeScript error:

Argument of type '"inexistant"' is not assignable to parameter of type '1 | "length"

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:

constm=Map<string,string|number>({length:3,1:'one'});

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:

typeMyMapType={length:number;1:string|null;optionalProperty?:string;};constm=Map<MyMapType>({length:3,1:'one'});

Are allMap methods implemented ?

For now, onlyget,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.

cmeiller, eesaber, and bsengupta-wavefin reacted with thumbs up emojikaelig reacted with heart emoji
@jdeniaujdeniau marked this pull request as draftJuly 15, 2021 14:29
@mbullington
Copy link
Contributor

Working today on seeing ifgetIn can be improved or implementing the other functions

jdeniau reacted with hooray emoji

@mbullington
Copy link
Contributor

mbullington commentedJul 16, 2021
edited
Loading

@jdeniau I spent a lot of time ongetIn but finally(!) found a solution using a new TS feature called variadic tuple types:microsoft/TypeScript#39094

Mind if I have write access to your fork so I can push my changes?

cypherfunc reacted with thumbs up emojijdeniau reacted with rocket emoji

@mbullington
Copy link
Contributor

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.

@jdeniau
Copy link
MemberAuthor

@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 likeMap(List(List(['a', 'A']))), but the worst thing for me is theObjectLikeMap interface, which seems really weird.
The best solution for me would be to have a uniqueinterface Map that handle both key/value sequence and also the object constructor, but I didn't handle that without breaking actual interface.
If we keep the two different interfaces, then all other method should be declared and are missing here (especiallyset andsetIn), but can be declared later.

@mbullington
Copy link
Contributor

mbullington commentedJul 19, 2021
edited
Loading

Was able to push my changes.

Thinking aboutObjectLikeMap, I think it's hard to integrate them because it's limited tostring | number, while Map can have arbitrary key values.

What's the issue withMap(List(List(['a', 'A'])))? Haven't tested it but curious which constructor does it use?

@jdeniau
Copy link
MemberAuthor

jdeniau commentedJul 20, 2021
edited
Loading

Nice implementation ! One think I'm not sure about (and something I did added) is theGetMapType<S> that is not generic and could not be reused for Record, List, etc. as it is tied to theObjectLikeMap, but your implementation is great : it does simplify a LOT thegetIn function.

AboutMap(List(List(['a', 'A']))), IIRC, It should matchMap(collection: Iterable<[K, V]>) but it doesn't asList(['a', 'A']) does not match the tuple[K, V] (as it's a List).

@jdeniau
Copy link
MemberAuthor

@bdurrer I will open a discussion or a wiki page once this is merged to keep the CHANGELOG light

bdurrer reacted with heart emoji

@mbullington
Copy link
Contributor

mbullington commentedNov 5, 2021
edited
Loading

Thanks all and@jdeniau for working on this!

I can understand the argument thatMap isn't the place for these and wanting to keep the typing aligned with the ES6 Map, so it's not without a good discussion.

I think the main goal of this is to enable strong typing for the "immutable POJOs," which in a lot of cases due tofromJS is how people use Immutable.@acusti There's no ES6 Map equivalent for that.

Right now it's hard to justify using the Immutable typing withfromJS since it'll sometimes get in the way, in the sense of "I know this is astring but I have to check because TS says it's astring | number" So this PR makes a world of difference there.

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 typesfromJS andtoJS, as well as adding support forupdate.

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

// https://github.com/immutable-js/immutable-js/issues/1462#issuecomment-584123268

/** @ignore */
type GetMapType<S> = S extends MapFromObject<infer T> ? T : S;
Copy link
Collaborator

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?

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

Copy link
MemberAuthor

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>;
Copy link
Collaborator

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>>(
Copy link
Collaborator

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

Copy link
MemberAuthor

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;
Copy link
Collaborator

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

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?

Copy link
MemberAuthor

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; }'.

* 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;
Copy link
Collaborator

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

Comment on lines +63 to 68
// @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');
Copy link
Collaborator

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?

Comment on lines 86 to 87
// $ExpectError
Map({ a: 4 }).get('b');
Copy link
Collaborator

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?

Map({ a: 4, b: true }).getIn(['a']);

// $ExpectType number
Map({ a: Map({ b: Map({ c: Map({ d: 4 }) }) }) }).getIn(['a', 'b', 'c', 'd']);
}
Copy link
Collaborator

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?

Copy link
MemberAuthor

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');
Copy link
Collaborator

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?

Copy link
MemberAuthor

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
Copy link
Contributor

mbullington commentedDec 1, 2021
edited
Loading

I took a stab at making suremerge & co work properly.


@leebyron Thanks for looking and identifying these issues withgetIn.

In honesty I changingnever tounknown would be best here, and keeping the current object-all-the-way-down path as helpful but not stopping you from usinggetIn otherwise.

My reasoning is we're dealing with both string literal types like"car" and then generic types likestring. With the[...T] syntax (microsoft/TypeScript#39094) I had to make itReadonlyArray<string | number | symbol> to get the path type looking like["a", "b", "c"] using literals. Otherwise it would use[string, string, string], so when we check ifstring extends "a" | "b" | "c", it doesn't and returns never.

This could present issues with typinggetIn for non-object-all-the-way-down structures.


I wonder if there's a better way of typinggetIn, I wonder if there's TS office hours or someone we could ask given Immutable's popularity as a project.

@eesaber
Copy link

Hi@mbullington

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 oftest isnumber[]

Playground Link

For ImmutableJS, I guess we could replaceT[U] by something likeT.get(U).

@jbaca303
Copy link

What's the status on this? Looks like it would be highly beneficial for a project my team is working on

@jdeniau
Copy link
MemberAuthor

@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
Copy link

@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.

Thanks for the quick update. Totally understand that this complex. Thank you guys for working on this, really appreciate it!

jdeniau reacted with heart emoji

@jdeniaujdeniau changed the base branch frommain to5.xAugust 24, 2023 09:56
@jdeniaujdeniau added this to the5.0 milestoneAug 24, 2023
@jdeniau
Copy link
MemberAuthor

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.

jbaca303 reacted with hooray emoji

@jdeniaujdeniau merged commitc25fac2 intoimmutable-js:5.xAug 24, 2023
@jdeniaujdeniau mentioned this pull requestAug 24, 2023
This was referencedJan 29, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@leebyronleebyronleebyron left review comments

@mbullingtonmbullingtonmbullington left review comments

@EduardoAraujoBEduardoAraujoBEduardoAraujoB left review comments

@bdurrerbdurrerbdurrer requested changes

Assignees
No one assigned
Labels
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.

Better Type Inference
9 participants
@jdeniau@mbullington@EduardoAraujoB@bdurrer@comatory@acusti@eesaber@jbaca303@leebyron

[8]ページ先頭

©2009-2025 Movatter.jp