Update Record Flow Type Definition

[ccorcos]

I added update and withMutations. The definitions for setIn, getIn, and updateIn are going to be a bit trickier.

Looking here for functions not included in the documentations: https://github.com/facebook/immutable-js/blob/master/src/Record.js#L131-L146

FYI this PR contains a commit from https://github.com/facebook/immutable-js/blob/master/dist/immutable.js.flow

[wokalski]

@ccorcos This repo is not actively maintained as far as I can see. I created a PR in flow-typed to add flow definitions for immutable there.

My fork is here. Feel free to submit a PR with your changes there.

[ccorcos]

@leebyron do you know if ImmutableJS is being maintained?

[ccorcos]

@wokalski thanks! I'll check em out

[tsemerad]

I'd love to see this implemented as well. I just setup Flow in my project, and it is counterproductive to keep seeing warnings that withMutations is not found in Record.

[lacker]

@ccorcos Yeah we are maintaining this now all seriously-like. I was thinking it would be nice when we update flow types to make sure they are exercised in a test (for the withMutations part) - does that seems sane? Or is that already happening somewhere.

[ccorcos]

@lacker flow-typed seems to be the new place to maintain 3rd party lib defs -- they have test setups and everything...

[lacker]

OK so #878 is in now so we have some Flow type refactorings and also you can now test flow types. Would you mind updating to merge those changes in, and also adding a test that catches these changes? Thanks!

[jedwards1211]

Here's something interesting I've been experimenting with in my own code. It's painfully verbose, but it typechecks extremely well.

// @flow

import {Record as iRecord} from 'immutable'

declare class RecordAPI<T> {
  constructor(init?: $Shape<T>): void;
  get<A>(key: $Keys<T>): A;
  set<A>(key: $Keys<T>, value: A): Record<T>;
  hasIn(keys: Array<any>): boolean;
  update<A>(key: $Keys<T>, updater: (value: A) => A): Record<T>;
  updateIn<A>(path: Array<any>, updater: (value: A) => A): Record<T>;
  merge(values: $Shape<T>): Record<T>;
  withMutations(mutator: (mutable: Record<T>) => any): Record<T>;
  inspect(): string;
  toObject(): T;
}

export function Record<T: Object>(spec: T): Class<RecordAPI<T>> {
  return iRecord(spec)
}

type AddressInit = $Shape<{
  street: string,
  city: string,
  zip: string,
}>

class Address extends Record(({street: '', city: '', zip: ''}: {
  street: string,
  city: string,
  zip: string,
})) {
  street: string
  city: string
  zip: string
}

type UserInit = $Shape<{
  name: string,
  address: AddressInit,
}>

class User extends Record(({name: '', address: new Address()}: {
  name: string,
  address: Address
})) {
  name: string
  address: Address
  constructor({name, address}: UserInit = {}) {
    super({name, address: new Address(address)})
  }
}

const user = new User({
  name: 'Andy',
  address: {
    street: 'wherever',
    city: 'Austin',
    zip: 'TX'
  }
})

user.set('name', 2)

console.log(user)
console.log('user.address instanceof Address:', user.address instanceof Address)

const user2 = new User({name: 'Bob'})

console.log(user2)
console.log('user2.address instanceof Address:', user2.address instanceof Address)

[wokalski]

Feel free to submit a PR if you have something working.

[rgbkrk]

Should this be rebased or closed?

I'd love to see the definition for Record's constructor (and the class it returns) improved in flow.

[wokalski]

I'd say closed and rewritten

[kozlitinaelja]

@ccorcos, from conversation above, it looks like this PR should be closed. Let me know if it is not correct assumption. 😅

[rgbkrk]

Thanks for responding and tagging this issue @kozlitinaelja!

It would probably help the community a lot if we could get the flow definitions that are shipped with this package moved out into flow-typed so we have a path to keep them updated together (or even to allow us to change by hand in our flow-typed directory).

[ccorcos]

@kozlitinaelja this PR is based off of #961 which simply defines types for the module default exports. So in fact, its only a 2 line change adding type definitions to Record for update and withMutations.

The other PR you mention does not fix these PRs and only tests the existing functionality. For example Record is a TODO: https://github.com/facebook/immutable-js/pull/878/files#diff-167148588528b5fa451dc535e9c29037R429

[leebyron]

@ccorcos could you remove the unrelated changes of handling exports? We should tackle that in a different PR. Also, please add some tests (see type-definitions/tests) that describe the correct behavior.

[jedwards1211]

@wokalski unfortunately, mystifying Flow errors occasionally occur when I use my RecordAPI type...I'm not sure if Flow object intersections still have bugs, but it sure seems like it.

The other problem with turning my suggestion into a PR is it's so verbose to use and I'm not sure I can make it work for both class extension and non-class extension usage of Record(). Any thoughts?

[wokalski]

@jedwards1211 I'd say let's wait for the conclusion of this PR.

@rgbkrk I started doing it in a PR to flow-typed but dropped it in favour of defs built in here. Maybe it needs to be revisited. You get many nice things with flow-typed like versioning. My line of thinking is that it allows more people to use flow defs. They don't need to use yet another tool.

[leebyron]

Closed by #1073

[cgestes]

#1073 seems to be related to TS.

Records are still missing most of the definitions in flow:)

[leebyron]

Yeah my mistake on typescript vs flow specific. I'll get equivalent changes out to flow types next :)

[cgestes]

Lovely. :)

Just in case its needed and you are not aware of it already, there is this gist on the subject:
https://gist.github.com/glenjamin/75a96b45f4bb5c6ac221815d28c548dd