Add union type

[Elendev]

To be able to test the union on swapi-graphql I've added a new MachineType type.

Machine is a union of VehicleType and StarshipType.
It comes with the machine and allMachines queries.

I'm not sure about the relevance of the name machine, so feel free to suggest an other name.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[IvanGoncharov]

I'm not sure about the relevance of the name machine, so feel free to suggest an other name.

I think spacecraft is the most generic name for something flying in outer space.
https://en.wikipedia.org/wiki/Spacecraft

To be able to test the union on swapi-graphql I've added a new MachineType type.

I also think it's valuable to have atleast one union type in sample API. At the same time I think this API should also be a sample on how you design your schema and in that sense it better to make MachineType an interface since VehicleType and StarshipType sharing so many fields. Plus having allMachines, allStarships and allVehicles looks redundant.

How about adding featuredConnection field to Film type that will return all People, Planet, Vehicle, etc. that were featured in this film?

{
  film(filmID: 1) {
    featuredConnection {
      featured {
        ... on Person {
          name
        }
        ... on Planet {
          name
        }
      }
    }
  }
}

@Elendev What do you think?

[Elendev]

I think spacecraft is the most generic name for something flying in outer space

I agree with you, but the thing is the vehicules doesn't all fly in outer space (ex. the X-34 landspeeder)

make MachineType an interface since VehicleType and StarshipType sharing so many fields

It's a good idea. Should I exclude the cargoCapacity and costInCredits fields from the interface (since they have the same name but not the same type) or should I refactor the Vehicle or the Starship types to standardize the types ?

You do have a point on the featuredConnection field on the Film type, it's a good place for an union.

[IvanGoncharov]

should I refactor the Vehicle or the Starship types to standardize the types?

@Elendev It makes a lot of sense to have standardized fields in all types.
Can you please make separate PR for this change.
Looking through cache.js it looks both fields always have integer values.
BTW, if you see inconsistencies in other places feel free to open a PR.

Should I exclude the cargoCapacity and costInCredits fields from the interface

I thought your intention for this PR was to add union type to this API:

To be able to test the union on swapi-graphql I've added a new MachineType type.

I think we need to keep example API as simple as possible and add new types/fields only if we really need to. So fully agree with adding new union type but I don't think we need another Interface type since we already have Node.

Another option for union type would be allMachines returning VehicleType, StarshipType and Person (only if it's Droid).

[Elendev]

I thought your intention for this PR was to add union type to this API

Yes, it was more a general question than a question for this specific PR.

Another option for union type would be allMachines returning VehicleType, StarshipType and Person (only if it's Droid).

@IvanGoncharov it's a good idea and it doesn't make me rewrite everything so I went with that.

[IvanGoncharov]

@Elendev I will try to review your code in the next couple of days, feel free to ping me otherwise.

[IvanGoncharov]

@Elendev Sorry for the delay. I'm working on cleaning up code base and making this project more Windows-friendly and it's taking more time than I estimated initially.
I would try to review your PR in the begging of the next week.

[Elendev]

@IvanGoncharov no problem, thank you for the heads up

[Elendev]

@IvanGoncharov ping

[IvanGoncharov]

Please copy updated header from master

[IvanGoncharov]

Both lines not needed after recent changes in master

[Elendev]

I've udpated with the last version of master and now the flow checks fails.

I get the following error :

Cannot call connectionDefinitions with object literal bound to config because GraphQLUnionType [1] is incompatible with
GraphQLObjectType [2] in property nodeType.

     src/schema/index.js
      79│  */
      80│ function rootConnection(name, swapiType) {
      81│   const graphqlType = swapiTypeToGraphQLType(swapiType);
      82│   const { connectionType } = connectionDefinitions({
      83│     name,
      84│     nodeType: graphqlType,
      85│     connectionFields: () => ({
      86│       totalCount: {
      87│         type: GraphQLInt,
      88│         resolve: conn => conn.totalCount,
      89│         description: `A count of the total number of objects in this connection, ignoring pagination.
      90│ This allows a client to fetch the first five objects by passing "5" as the
      91│ argument to "first", then fetch the total count so it could display "5 of 83",
      92│ for example.`,
      93│       },
      94│       [swapiType]: {
      95│         type: new GraphQLList(graphqlType),
      96│         resolve: conn => conn.edges.map(edge => edge.node),
      97│         description: `A list of all of the objects returned in the connection. This is a convenience
      98│ field provided for quickly exploring the API; rather than querying for
      99│ "{ edges { node } }" when no edge data is needed, this field can be be used
     100│ instead. Note that when clients like Relay need to fetch the "cursor" field on
     101│ the edge to enable efficient pagination, this shortcut cannot be used, and the
     102│ full "{ edges { node } }" version should be used instead.`,
     103│       },
     104│     }),
     105│   });
     106│   return {
     107│     type: connectionType,
     108│     args: connectionArgs,

     node_modules/graphql-relay/lib/connection/connection.js.flow
 [2]  63│   nodeType: GraphQLObjectType,

     src/schema/relayNode.js
 [1]  22│ ): GraphQLObjectType | GraphQLUnionType {

First I thought that the last update of graphql-relay broke something, but the flow definition didn't changed, as we can see here for the previous version : https://github.com/graphql/graphql-relay-js/blob/v0.5.1/src/connection/connection.js#L64

@IvanGoncharov have you already met the same kind of issues or have you any idea on how to fix this ? I have to admit I don't know why it worked before and it's not working anymore.