joarwilk / gql2flow

Convert a GraphQL Schema to a flow definition
97 stars 24 forks source link

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

Closed capaj closed 7 years ago

capaj commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

Whats the reason for not using | null?

joarwilk commented 7 years ago

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).