joarwilk/gql2flow

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

Closed this issue · 7 comments

capaj commented

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

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.

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
}

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,
}

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

@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

Whats the reason for not using | null?

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