Improve scalability of Immutable.Map TS types

[mbullington]

Hello! @immutable-js/maintainers @Methuselah96

We currently use Immutable.js pretty extensively in a Redux store and are progressively moving to TypeScript. In trying to strongly type our Immutable.Map objects, I settled for the time being on this:

export type FontUpload = Immutable.Map<
  'name' | 'created' | 'errorMessage' | 'progress' | 'complete',
  never
>;

It's less than ideal for strongly typing our stores, and given the option something like this would be great:

export type FontUpload = Immutable.Map<{
  name: string,
  created: number,
  errorMessage?: string,
  progress?: number,
  complete?: boolean
}>;

Is this something Immutable.js would be interested in for the v4.0.0 release? Happy to help with this work as long as devs find it reasonable.

I may need some TS help with overloading generics, if that's even possible. Would anyone know about the feasibility of this?

Best,

[Methuselah96]

Is there a reason you're not using Records instead of Maps? Records seem like a more appropriate data structure for what you're trying to accomplish if I understand your use case correctly.

[jelder]

@Methuselah96 Record is indeed a good way to handle this, however, there are cases where Map has to be preferred.

The signature for Record.update is lacking some overloads compared to Map.update:

update<K>(key: K, updater: (value: TProps[K]) => TProps[K]): this

vs:

update(key: K, notSetValue: V, updater: (value: V) => V): this
update(key: K, updater: (value: V) => V): this
update<R>(updater: (value: this) => R): R

Significantly, the 3rd overload of Map.update receives this and so can access other properties of the map. If Record.update also had this overload, it could eliminate a lot of uses of less-safely-typed Map for us.

[cypherfunc]

My team had/has the same issue; Records didn't exist when we started using Immutable, so turning objects into Maps was the only choice. We also weren't using TS yet, so this wasn't a problem. I can tell you from experience that translating a Map-based code base into Record-based takes time, but the effort is pretty straightforward and definitely worth it if you can afford the person-hours.

As for the types themselves, the best we could come up with is Map<string, unknown> or Map<keyof Thing, unknown>, and then casting the value each time you pull out a property. Conceptually, Maps are key-value dictionaries, so it doesn't make mathematical sense for them to have assigned property types (as much as we might want them in practice!) Records really are the way to go here.

The frustrating bit which could probably be improved is that during the switch from using Map to using Record, there are many places that can accept either, and use them the same way at runtime, but the types don't match. e.g.:

const selectName = (person: Map<keyof Person, unknown>|Record<Person>) => {
    const name1 = person.get("name"); // TS doesn't like this, the type of Map get doesn't match Record get!
    const name2 = Map.isMap(person) ? person.get("name") : person.get("name"); // this kind of code generally works
    return name2;
};

[cypherfunc]

Significantly, the 3rd overload of Map.update receives this and so can access other properties of the map. If Record.update also had this overload, it could eliminate a lot of uses of less-safely-typed Map for us.

Interesting. My team hasn't run into this, we generally use .merge or .set to make any changes to Records. I didn't even realize Records didn't have that overload, it seems like it'd be useful.

[cypherfunc]

(I'm not claiming to be an authority on Immutable, just a humble user with a few years experience, during which this Map vs Record stuff has caused plenty of headaches.)

[Methuselah96]

Thanks for the input everyone! Sounds like one actionable item is updating the Record#update types to allow for the different overloads. PRs are welcome to fix that issue.

[JustFly1984]

Ad I remember, the only performance optimized immutable data structures is Map and List. I've abandoned immutable.js 3 years ago in all of new projects due to unmaintained status of the repo and no proper support for typescript. As I remember I had an issue with a List, where I had issues with order of elements in Lists bigger than 32 elements, resulting in incorrect iteration over List with 33 elements. I had to convert List to native JS Array with .toJS() which is a huge perf bump.

[jdeniau]

Hi @mbullington

There is a PR opened on the fork we made : immutable-js-oss#209

It fixes some issues listed here (not the update though, but some of the get and constructor issues)
Feel free to review and test it and I will reopened it here

[jdeniau]

The PR has been moved in this repository : #1841 or #1837

[jdeniau]

To be more specific : @mbullington your first solution is implemented by #1837 and released in 4.0.0-rc.14. The other solution is in #1841, but it is not ready to be merged right now.
If you are good in TS, I do appreciate any help there, especially for the getIn function.

I'm closing this

[mbullington]

I'll try taking a look. Thanks a ton! @jdeniau

[jdeniau]

Closing as we are now talking in #1841