pbomb / flow-immutable-models

Generates model classes from Flow types using Immutable.js
42 stars 8 forks source link

Clarify the specs regarding extra properties in objects passed to fromJS() #24

Open dperetti opened 7 years ago

dperetti commented 7 years ago

When we instantiate a model, it's critical to make sure not to use properties that are not part of the model type, especially because it can be the result of a typo.

For example, given the definition:

export type BarModelType = {
  barStr: string,
  barNum: number,
};

This should yield an error:

const bar = Bar.fromJS({ barstr: "blah" })

because barstr !== barStr.

It's not clear whether the library is supposed to allow Flow to detect these errors. Currently, sometimes it does not, and sometimes is does but it seems to be a side effect of the use of $Shape<> in some condition (when default properties are defined). I'm not 100% sure about this, but I think this requirement (no extra properties) could be precisely enforced by prepending $Shape<BarModelType> & ... to the json parameter of the fromJS(). method.

dperetti commented 7 years ago

Update :-)

The auto-generated code in the bar.js example is not current. When using the current version, this line is generated, which seems to implement what I suggested above.

type BarFromJSType = $Shape<BarFullType> & BarRequiredArguments;

However, why not simply use the following instead ?

type BarFromJSType = $Shape<BarModelType> & BarRequiredArguments;

It seems to help flow better at showing where the error actually is.

pbomb commented 7 years ago

What are your default values in this case? I'm curious why BarFullType and BarModelType behave differently.

dperetti commented 7 years ago
export type BarModelType = {
  barStr: string,
  barNum: number,
};

export const defaultBarValues: BarModelType = {
  barStr: 'foo',
  barNum: 3,
}

const b = Bar.fromJS({
  a: "mklj",
})

Using:

type BarFromJSType = $Shape<BarFullType> & BarRequiredArguments;

Flow yields:

examples/bar.js:59
               v-----------
 59: const b = Bar.fromJS({
 60:   a: "mklj",
 61: })
     -^ call of method `fromJS`
 56: type BarFromJSType = $Shape<BarFullType> & BarRequiredArguments;
                                 ^^^^^^^^^^^ BarFullType. Property cannot be assigned on any member of intersection type
 56: type BarFromJSType = $Shape<BarFullType> & BarRequiredArguments;
                                 ^^^^^^^^^^^ intersection
  Member 1:
   55: type BarFullType = BarOptionalArguments & BarRequiredArguments;
                          ^^^^^^^^^^^^^^^^^^^^ BarOptionalArguments
  Error:
                            v
   59: const b = Bar.fromJS({
   60:   a: "mklj",
   61: })
       ^ property `a` of object literal. Property not found in
   55: type BarFullType = BarOptionalArguments & BarRequiredArguments;
                          ^^^^^^^^^^^^^^^^^^^^ object type
  Member 2:
   55: type BarFullType = BarOptionalArguments & BarRequiredArguments;
                                                 ^^^^^^^^^^^^^^^^^^^^ BarRequiredArguments
  Error:
                            v
   59: const b = Bar.fromJS({
   60:   a: "mklj",
   61: })
       ^ property `a` of object literal. Property not found in
   55: type BarFullType = BarOptionalArguments & BarRequiredArguments;
                                                 ^^^^^^^^^^^^^^^^^^^^ object type

Which is quite complicated and confuses both Webstorm and Sublime with Flow linter.

As opposed to using:

type BarFromJSType = $Shape<BarModelType> & BarRequiredArguments;

... where Flow just yields:

examples/bar.js:59
               v-----------
 59: const b = Bar.fromJS({
 60:   a: "mklj",
 61: })
     -^ call of method `fromJS`
                          v
 59: const b = Bar.fromJS({
 60:   a: "mklj",
 61: })
     ^ property `a` of object literal. Property not found in
 56: type BarFromJSType = $Shape<BarModelType> & BarRequiredArguments;
                                 ^^^^^^^^^^^^ object type

... which is way simpler and makes IDEs happy !

pbomb commented 7 years ago

Thanks for the detailed writeup. I'm going to be out the rest of the week, but will try to get this fixed next week.