[WIP] Definitions for immutable

[wokalski]

This is work in progress. One test fails and there are a few undefined classes.
There are a few related PRs in the immutable repo.

[verekia]

Thanks @wokalski for taking care of Immutable. Will your PR help with immutable-js/immutable-js#863 by any chance?

[wokalski]

@verekia

Good question! The problem with this issue is that it requires some changes to Immutable too. Which is not actively maintained...

I didn't consider the issue personally but it looks like we would need to change the exports to match with Flow's source (only include default export). It might break many applications (including mine) but I'd still do it.

[verekia]

Okay, that's too bad :)
Thank you for looking into it!

[iansinnott]

Really great to see someone working on this 👏 . The official types that ship with Immutable currently have some issues, especially with OrderedMaps (immutable-js/immutable-js#979) and Records (immutable-js/immutable-js#998).

[karelbilek]

hah, I just wanted to ask about importing this correctly and how does it work.

I do not want to overwrite global Map. import {Map} from 'immutable'; works, but not for this file.

When I rewrite the flow file a bit, it works. I will send you a pull request to the pulll request. :)

[wokalski]

OK, it looks like there's not going to be too much activity here. I'll fix the existing failures and then remove the WIP and try to merge it.

[karelbilek]

oh nevermind, it was my own issue. :) sorry. Thanks for this PR!!!

[wokalski]

@Runn1ng OMG, I didn't notice the last paragraph. Thanks ❤️ . If you sent me the PR I'd be super happy. Thanks a lot and sorry for the douchebaggery but I didn't copy the last paragraph.

[karelbilek]

My problem is that everything is exported to "global" immediately, instead of being "hidden" in the module by

declare module 'immutable' ...

I know that's in the original definition in the immutable repo and I don't like it there either. So I wanted to close it in declare module 'immutable'.

[karelbilek]

However, I am not sure if this is a good idea and how will these clash with the built-in immutable definitions.

[karelbilek]

Just for interest. I have made for myself a Record class wrapper, that checks Records for me. It's not complete, but it checks the types of the parameters, which is what I wanted; I don't use getIn/ get/set/setIn.

The small wrapper looks like this

/* @flow */

import {Record as ImmutableRecord} from 'immutable';

export type Record<Def: Object> = {
  merge: (o?: $Shape<Def>) => Record<Def>;
} & Def;

type $RecordCreator<Def: Object> = (def?: $Shape<Def>) => Record<Def>;

export function RecordC<Def: Object>(definition: Def): $RecordCreator<Def> {
  const RecordClass = ImmutableRecord(definition);
  return (newDef) => {
      return new RecordClass(newDef);
  };
}

You can only use the "default" getters, but it checks them correctly; you cannot use set, you can only use merge, but it checks the types correctly (unlike using set with this definition). I use it like this

/* @flow */

import type {Record} from './record'
import {RecordC} from './record'

const ABRecordC = RecordC({a: 3, b:3});
const recW = ABRecordC({b: 2});
recW.merge(); //works
recW.c; // throws error
recW.a; 

// or explicitly
type CDRecord = Record<{c: number, d: number}> 
const CDRecordC = RecordC({c: 3, d:3});
const rec: CDRecord = CDRecordC();
rec.merge();
rec.merge({c: 3});
rec.merge({c: 'foo'}); // throws error
rec.c;
rec.a; // throws error

I have to do the dance with new because of this - facebook/flow#2930

It works as I want to. You can use it if you want

[wokalski]

Just to be clear, what does this wrapping mean? I don't get the idea behind it. Does it hide the default export from flow? Or the opposite?

These probably override whatever is inside the built in defs, don't they?

Let's sort out this issue as it's the only one that's stopping this PR except for one test which doesn't pass.

I will use the definition, sure.

[karelbilek]

You can use it instead of the default defs; and it implements only subset of the Record capabilities; but I am using it, because it does more typechecking.

I don't think you should add it to the file, since other people can use get/set and it will break their code. I added it here just for interest.

[wokalski]

So last question is what does the declare module do ?

[wokalski]

I'm closing this one because proper flow definitions were merged in immutable-js.