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

question-wouldn't it make more sense to place the "?" on a property key? #12

Closed
capaj opened this issue Mar 19, 2017 · 7 comments
Closed

Comments

@capaj
Copy link

capaj commented Mar 19, 2017

most types are generated as these two:

 description: ?string;
 target: ?any;

wouldn't it make more sense to generate them rather as:

 description?: string;
 target?: any;

Since we're always "picking" props we're expecting from the backend in graphql?

@kitze
Copy link

kitze commented Mar 20, 2017

I think this is a bug. It should be as @capaj is saying. If the property is there, it should be of type "string", but if it's not in the object, Flow should be fine with that object.

@joarwilk
Copy link
Owner

Hmm, I'm not sure here. I think having the value as null makes sense

// GraphQL Schema
User {
  id: ID!
  name: String
}

... + some user query

// GraphQL Query
{
  user(id: 123) {
    id
    name
  }
}

// result if name is not set
{
  "user": {
    "id": "123",
    "name": null
  }
}

// Matching flow definition
type User {
  id:  string,
  name: ?string
}

@petrbela
Copy link

petrbela commented Mar 30, 2017

Well, technically, the right way to do this would be to have all properties optional (since it's up to the client to only request what they want), and those that can be null should be maybe types.

description?: ?string;
target?: ?any;

Or, in the case of user:

type User {
  id?: string,
  name?: ?string,
}

@joarwilk
Copy link
Owner

joarwilk commented Mar 30, 2017

Yep, thats the proper solution @petrbela. Thanks, will implement.

Edit: although correct it might be a bit too verbose on the maybe types, will be somewhat frustrating to work with. I'll add all three variations and a cli flag to choose with.

@kitze
Copy link

kitze commented Apr 14, 2017

@joarwilk I would love to see that, right now if i need to update 1 property i need to set all of the other ones to "null" just for Flow to work.

This is an awesome project and thanks for your great work!

@gajus
Copy link
Contributor

gajus commented Apr 18, 2017

Whats the reason for not using | null?

@joarwilk
Copy link
Owner

joarwilk commented May 2, 2017

It now defaults to making no assumptions about the usage and then you can use it how you'd like with --null-keys and --null-values (v0.4.0).

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

5 participants