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

feat(gatsby-source-wordpress): MediaItem.excludeFieldNames / auto exclude interface types that have no fields #37062

Merged
merged 10 commits into from
Nov 21, 2022

Conversation

TylerBarnes
Copy link
Contributor

@TylerBarnes TylerBarnes commented Nov 18, 2022

This allows the options.type.MediaItem.excludeFieldNames to be able to exclude field names on the MediaItem type during sourcing.

While fixing this I discovered interface types that have no fields due to an implementing type having every field on that interface excluded needed to be auto excluded so that this fix doesn't cause build errors.

I released a canary as gatsby-source-wordpress@7.2.0-alpha-wordpress-exclude-fields-on-media-item.14

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 18, 2022
@TylerBarnes TylerBarnes removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 18, 2022
@@ -1493,7 +1493,6 @@ Array [
Object {
"fields": Array [
"avatar",
"databaseId",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because this field was already excluded in the test site but due to a bug that this PR happens to fix it's now properly excluded in the schema.

"template",
],
"name": "WpNodeWithTemplate",
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These snapshot changes are because I added options.type.MediaItem.excludeFieldNames: [template] in the test site gatsby-config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WpNodeWithTemplate is now automatically excluded because one of the types that implement that interface (WpMediaItem in this test site) has excluded the only field that interface defines: template.
Any type that implements an interface must have every field in that interface. So to prevent errors, this type must also be excluded as a result of excluding MediaItem.template.

// if this is a union type, make sure the union type has one or more member types, otherwise schema customization will throw an error
(!!type.possibleTypes && !!type.possibleTypes.length)
)
const implementingTypes = getTypesThatImplementInterfaceType(type)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I abstracted all this code into getTypesThatImplementInterfaceType because it needs to be run during query generation as well as schema customization, where before it was only needed during schema customization.

Comment on lines -77 to -79
gatsbyNodeTypes,
fieldAliases,
fieldBlacklist,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transformFields now gets these values internally to make it easier to use transformFields in multiple places (which this PR is doing now)

return (
!interfaceTypeSettings.exclude &&
fieldOfTypeWasFetched(type) &&
fieldOfTypeWasFetched(interfaceType)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interface types that were not fetched (added during query building) shouldn't appear in the schema, so this filters them out

@@ -84,7 +116,7 @@ export const typeIsASupportedScalar = type => {
return supportedScalars.includes(findTypeName(type))
}

const typeSettingCache = {}
const typeSettingCache = new Map()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use a Map for caching since this function is called tons of times during schema customization and query building

Comment on lines -65 to -66
* custom scalar types aren't imlemented currently.
* @todo make this hookable so sub-plugins or plugin options can add custom scalar support.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not going to do this so I removed the todo

parentInterfacesImplementingTypes: typesThatImplementInterface,
})

if (shouldSkipInterfaceType && interfaceType.name !== `Node`) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We always need the WpNode type no matter what and it causes other problems if it's auto-excluded

Comment on lines +31 to +34
const parentTypeNameSettings = getTypeSettingsByType(parentType)
const parentTypeNodesFieldTypeNameSettings = getTypeSettingsByType(
parentTypeNodesField?.type
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the proper way to look up plugin options type settings, not sure why I did it the other way before ;p

Comment on lines +346 to +355
cachedMediaItemNodeIds.map(async nodeId => {
const node = await helpers.getNode(nodeId)

const parentNode =
node?.internal?.type === `File` && node?.parent
? helpers.getNode(node.parent)
: null

return parentNode || node
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this was a separate bug where we were returning the wrong kind of node since the code surrounding this expects MediaItem nodes, not File nodes

Copy link
Contributor

@kathmbeck kathmbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! The comments were really helpful.

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

Successfully merging this pull request may close these issues.

4 participants