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

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

Open
dperetti opened this issue Jul 25, 2017 · 4 comments

Comments

@dperetti
Copy link

dperetti commented Jul 25, 2017

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
Copy link
Author

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
Copy link
Owner

pbomb commented Jul 25, 2017

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

@dperetti
Copy link
Author

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
Copy link
Owner

pbomb commented Jul 25, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants