Share union type instances

[haroldangenent]

Hello, Gatsby team 👋

I hope to get some feedback on this PR. Let's dive in. Currently, union types are named as follows:

1. Using the names of all the fields they contain;
2. Incremented automatically when the same key occurs (createTypeName).

This causes union types to not be very reliable for usage in GraphQL queries (and not pretty, such as unionContentPostNodeWordPressAcfImageWordPressAcfText_2 for a WordPress flexible content field).

This issue is mentioned in #4074 (comment) and is the root of the cause of this problem: #8615. This PR takes a first shot at naming union types more consistently.

• Naïvety: Might this approach be overlooking some use-case in which you would not want to share union type instances when the same child-node types are used? I'm not too familiar with the use-cases for all source plugins.
• Shared across all keys: In this PR, union type instances are shared across all fields that contain the same child-node types, disregarding the key of the field. We could also choose to only share these instances for the exact same field key, to name union types more consistently for some specific use-cases. Any ideas on this?
• createTypeName and updated schema: Because createTypeName is used (that stores all seen names in memory), the field name still gets incremented. This ensures a unique union type, when the same key is used for different child-node types. However, because schema is updated during bootstrap (for the SitePage node), all fields using createTypeName end on _2. While this never changes, this makes it seem less reliable for developers trying to use the field. Maybe this is out of scope, but would there be a way to fix this issue? If you have any pointers on where to start, that would be great.
• Test: I could also use some pointers on how to write a test for this use-case. Looking at the current tests, I'm not sure on how to check the type of a union type field or even query several node types (only listNode is queried).

Thanks!

[pieh]

let's make unionTypes an object/map so we don't need to find type in array.

map keys could be name of type, so first thing here is to generate typeName:

const typeName = `Union_${key}_${fields.map(f => f.name).sort().join(`__`)

check if that typeName is already in our map and return it of it is

if it's not create type and set our new type in our map

[haroldangenent]

I've made unionTypes an object, so union type instances can be shared on a per-key basis. The value still needs to be an array though (and we still need to use .find on it), because of this use-case:

• A node type contains a union type on the field with the key content;
• Another node type also contains a union type on the content field, but it contains different node types for the children.

In this case, we do not want to share the union type. We want to verify that it actually contains the same node types.

In case you hadn't already noticed, I've changed the name of the union type to exclude all fields names. This way the union type name doesn't change when you the data structure for the children change (in case of WordPress: adding or removing an ACF layout).

[pieh]

Right, that's why object/map key would need to include field type names and not just field key.

[haroldangenent]

I've been thinking about this, but I think there's an issue with using the field type names as a key. The reason behind this, is that the schema is updated when bootstrapping Gatsby. On bootstrap, the schema is re-created.

If we are using the field names as a key, the same union type instance that is created during initial schema creation, is also used when the schema is updated. The problem here is that the union type defines which child node types it has, which are in turn registered as fields during schema creation (in GraphQLSchema).

Since the 'old' union type (and its child node types) are used, the schema tries to create the child node type for both the old union type and the new child node type during schema creation. Since GraphQL's JS implementation checks for type names being the exact same type (and compares the object itself instead of just the names), this error would be thrown.

This is why comparing the objects using _.isEqual does work, since they are not equal and a new union type is actually created when updating the schema (which it should). I think we have two options:

1. Resetting the unionTypes map when starting to construct schema (as you proposed for seenNames)
2. Keep comparing types using .find and _.isEqual

Do you have any other ideas? Which option has your preference?

[pieh]

We can clear that map when we start (re)building schema (in schema/index.js)

[haroldangenent]

Thanks for your reply. I've updated the PR and converted to a Map for ease of clearing the union types map.

[pieh]

I think this is very valid.

Let's start with problem of adding _2 when schema is updated:
We could reset seenNames map in https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/schema/create-type-name.js when we start constructing schema ( https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/schema/index.js ) - problem here is it would possibly be breaking change for some who rely on _2 type names for their fragments, so not sure if we should actually do this right now.

[haroldangenent]

Thanks for your feedback. I think you're right about the _2 change being backwards incompatible. Maybe not necessarily for union types, but definitely for all other types that are currently consistently named _2. Too bad. Is there any place to note this idea, for when a break in the code will happen anyway?

It might be worth noting that this PR will also break the use of union types in the same way, but they currently already break whenever the data structure is updated or whenever another type is unnecessarily created for a different node type (incrementing _2 to _4 or _6), rendering it unusable at the moment.

If you agree with the key-based approach, I think the only part left is to create a test for this change.

[haroldangenent]

@pieh Sorry for pinging you again, but if you have any tips on how to approach the test (preferably with multiple node types I think), I could do some work on that.

[pieh]

@haroldangenent Please, ping me whenever you need answers/help! (it's just sometimes hard for me to keep up with multiple open PRs, so definitely my bad)

this is very basic test that we can add - haroldangenent#1 additional tests would be good to check if same union type is being used if keys in field/field2 would be different would be different

[haroldangenent]

@pieh Thanks for your response and PR. Your test really helped me to get going, inferObjectStructureFromNodes was what I needed.

I've added tests for several cases, including consistent naming of the union type to ensure it doesn't break on a future (minor) release. In order to do test this properly, I've converted the createTypeName to a Map similar to unionTypes and provided a clearTypeNames function that's not called during schema creation (yet). Whenever a breaking change on type names is possible, this could also be done. For now it's used to make sure each test runs in isolation.

Let me know what you think!

[pieh]

provided a clearTypeNames function that's not called during schema creation (yet)

Yeah, we will definitely change that for next major release (as it will be breaking change) - in general a lot plans around schema improvements, but need to keep it as is for now :(

I think this looks good, will give it more thorough review tomorrow

[pieh]

Thanks @haroldangenent!

[gatsbot]

Holy buckets, @haroldangenent — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!