-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Definitions for immutable #401
Conversation
Thanks @wokalski for taking care of Immutable. Will your PR help with immutable-js/immutable-js#863 by any chance? |
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. |
Okay, that's too bad :) |
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). |
hah, I just wanted to ask about importing this correctly and how does it work. I do not want to overwrite global Map. When I rewrite the flow file a bit, it works. I will send you a pull request to the pulll request. :) |
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. |
oh nevermind, it was my own issue. :) sorry. Thanks for this PR!!! |
@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. |
My problem is that everything is exported to "global" immediately, instead of being "hidden" in the module by
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 |
However, I am not sure if this is a good idea and how will these clash with the built-in immutable definitions. |
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 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 /* @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 It works as I want to. You can use it if you want |
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. |
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 |
So last question is what does the declare module do ? |
I'm closing this one because proper flow definitions were merged in |
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.