Skip to content
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

Record definitions are sloppy #1722

Open
gryn opened this issue Jul 3, 2019 · 1 comment · May be fixed by #1870
Open

Record definitions are sloppy #1722

gryn opened this issue Jul 3, 2019 · 1 comment · May be fixed by #1870
Labels

Comments

@gryn
Copy link

gryn commented Jul 3, 2019

Flow version: 0.97.0 / immutable 4.0.0-rc12

What happened

When defining records, you have to do all the typecasting because Record() has a incorrect type definition.

How to reproduce

type Product = { address: string };
type App = { name: string };
const AppDefaults: App = { name: '' };

const AppRecordImplicit = Record(AppDefaults);        // class RecordInstance<T: any = any>
const AppRecordExplicit = Record<App>({ name: '' });  // class RecordInstance<T: any = any>
const AppRecordBorked: RecordFactory<App> =           // RecordFactory<App>
  Record<Product>({ address: '' });

const appImplicit = AppRecordImplicit({ name: 'i' }); // RecordOf<empty>
const appExplicit = AppRecordExplicit({ name: 'e' }); // RecordOf<empty>
const appBorked = AppRecordBorked({ name: 'b' });     // RecordOf<App>

In the above code, none of the ways of creating records work. The only way that actually works is to say:

const AppRecordExtraExplicit: RecordFactory<App> = Record<App>(AppDefaults);

Ideally, I'd expect either of the first two to work (AppRecordImplicit or AppRecordExplicit). However, they create RecordOf<empty> since they are not typed themselves (they receive class RecordInstance<T: any>).
The third call illustrates how that is a bit crazy, since you can successfully cast a record factory of one type to another (which leads you to be able to make records of the wrong type entirely).

Only by using AppRecordExtraExplicit -- and using it correctly, since it is the same form as AppRecordBorked -- can you get a RecordFactory<App> and produce actual RecordOf<App>.

@gryn
Copy link
Author

gryn commented Jul 3, 2019

I am not skilled at these type definitions, but it seems that:

declare class Record {
  static <Values: Object>(spec: Values, name?: string): typeof RecordInstance;
  constructor<Values: Object>(spec: Values, name?: string): typeof RecordInstance;

  static isRecord: typeof isRecord;

  static getDescriptiveName(record: RecordInstance<any>): string;
}

Needs to accept <T>, perhaps as simply as this:

declare class Record<T> {
  static <Values: Object>(spec: Values, name?: string): typeof RecordInstance<T>;
  constructor<Values: Object>(spec: Values, name?: string): typeof RecordInstance<T>;

  static isRecord: typeof isRecord;

  static getDescriptiveName(record: RecordInstance<any>): string;
}

mwiencek added a commit to mwiencek/immutable-js that referenced this issue Jul 31, 2021
This allows for better type inference of the resulting RecordFactory.

Fixes immutable-js#1722
@mwiencek mwiencek linked a pull request Jul 31, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants