toJS breaks type compatibility between records with object values

[thatsmydoing]

What happened

Records with object values that would otherwise be compatible are not because of toJS. I've noticed this when fields only differ by optionality.

How to reproduce

import { RecordOf } from 'immutable';

interface Id {}

type A = RecordOf<{ id: Id }>;
type B = RecordOf<{ id?: Id }>;

const a: A = null as any;
const b: B = a;

Results in the error

Type 'A' is not assignable to type 'B'.
  Type 'A' is not assignable to type 'Record<{ id?: Id | undefined; }>'.
    The types of 'set(...).toJS().id' are incompatible between these types.
      Type 'unknown' is not assignable to type 'Id | undefined'.

I think this happens due to the workaround for circular types where objects become unknown. Honestly, I think the new types are more trouble than they're worth and would prefer the 4.1 types.

[jdeniau]

Hi @thatsmydoing .

Thanks for the report. #1932 should fix this.

About the fact that you do prefer "old" types : there was unknown nearly everywhere, so you lose the benefits of TS really quicky.

I'm trying to improve the types one step at a time, but it might break sometime on some implementation, despite the fact that I add unit tests and I test it on a project with ~250 TS files and a lot of immutable code.

Feel free to report any issue you might have on TS types though, I will try to do my best to fix them as soon as possible.

[thatsmydoing]

About the fact that you do prefer "old" types : there was unknown nearly everywhere, so you lose the benefits of TS really quicky.

Indeed, but typescript is also about being pragmatic with types and I think the previous typing for toJS struck a good balance. I suppose I'm speaking for myself but I don't really expect toJS to return something fully typed. Where toJS is used, you're typically passing it to JSON.stringify or just lightly massaging it to be made as a payload for something. I don't expect to actually mutate the result or store it long term.

I do appreciate getting better typing in though, so if you can make it work that's great. But otherwise, I wouldn't mind just having more conservative types.

[jdeniau]

@thatsmydoing Can you try patching your local immutable definition file and test the fix to see if it does work for you?

[thatsmydoing]

The linked PR has fixed the toJS errors but there's a similar case that's broken with has()

import { RecordOf } from 'immutable';

export type Id = {};

type A = RecordOf<{ id: Id, param: boolean  }>;
type B = RecordOf<{ id: Id}>;

const a: A = null as any;
const b: B = a;

errors with

Type 'A' is not assignable to type 'B'.
  Type 'A' is not assignable to type 'Record<{ id: Id; }>'.
    Types of property 'has' are incompatible.
      Type '(key: string) => key is "id" | "param"' is not assignable to type '(key: string) => key is "id"'.
        Type predicate 'key is "id" | "param"' is not assignable to 'key is "id"'.
          Type '"id" | "param"' is not assignable to type '"id"'.
            Type '"param"' is not assignable to type '"id"'.

There's also a small part of our code where 4.1 -> 4.2 broke some generic type inference but I don't know if that can be reasonably fixed.

[thatsmydoing]

Hm, actually the has might be unrelated. I think it was the type inference that caused the error I was seeing. That said, if you change toJS to

    toJS(): {
      [K in keyof TProps]: DeepCopy<TProps[K]>;
    };

it fixes our type inference issues.

[jdeniau]

About has, I created #1934.

About your proposal, I will check that, but that exactly what DeepCopy do in the first case. The problem might come from RecordOf instead of Record (I did not know this type !)

[jdeniau]

As a matter of fact, I do test RecordOf in https://github.com/immutable-js/immutable-js/pull/1932/files#diff-a97aee554c7afd2dad1a12e820058a23afd07daccada6c22e392a35dd9923c81R108-R118

Can you maybe provide another minimal reproductible case that I can check ?

[thatsmydoing]

About your proposal, I will check that, but that exactly what DeepCopy do in the first case.

Indeed, but DeepCopy has a lot of other branches so it's probably best to have it go through as little infer as possible.

The type inference issue we ran into is:

import { RecordOf } from 'immutable';

type Child = RecordOf<{}>;

interface IBase {
  child: Child;
}

interface IA extends IBase {
  param: boolean;
}

type A = RecordOf<IA>;

function foo<P extends IBase>(record: RecordOf<P>): RecordOf<P> {
  return record;
}

const a: A = foo(null as unknown as A);

with the relevant error being

Type 'RecordOf<IBase>' is not assignable to type 'A'.
  Type 'RecordOf<IBase>' is not assignable to type 'Record<IA>'.
    The types returned by 'set(...).toJS()' are incompatible between these types.
      Property 'param' is missing in type '{ child: {}; }' but required in type '{ param: boolean; child: {}; }'.

What happens is that it's not successfully inferring P anymore and defaults to just IBase. It should be able to infer the types just from the input parameters and not result in an error.