feat: Allow themes to modify GraphQL types #16928

stefanprobst · 2019-08-21T20:19:15Z

Currently, a GraphQL type can only be modified by the plugin which created that type, or by a user's gatsby-node. This excludes a theme's gatsby-node.

This PR allows a theme's gatsby-node to modify types. A top-level gatsby-node always takes precedence. If a type has already been modified by another theme, modification is not allowed.

Fixes #16529
Fixes #15544

stefanprobst · 2019-08-21T21:02:41Z

apparently node 11 (or v8 really) has changed the sort implementation, which is why this doesn't work anymore:

gatsby/packages/gatsby/src/schema/index.js

Lines 42 to 44 in c2fc3ee

    
           const sortedTypes = types.sort( 
        
             type => type.plugin && type.plugin.name === `default-site-plugin` 
        
           )

wangyi7099 · 2019-08-23T03:42:45Z

packages/gatsby/src/schema/schema.js

    plugin.name === `default-site-plugin` ||
-    plugin.name === typeOwner
+    (plugin.name.startsWith(`gatsby-theme-`) &&
+      !typeOwner.startsWith(`gatsby-theme-`))


Just wonder that does the name of a theme have to start with "gatsby-theme" ? If so, I think that's a little unreasonable. My theme plugin's name is antdsite , not starts with "gatsby-theme". I think we should judge theme in another way.

the gatsby-theme- naming is a requirement since #16620, so we can rely on that. it's not totally optimal since i don't think it is actually enforced anywhere, otoh the only thing you'll lose (currently) when naming your theme differently is the ability to modify types owned by plugins. i'm open to other proposals, please post them in #16529.

Lol All right, since you haved discussed.

sorry, looks like my assumption was wrong

wardpeet

Code looks 👍 I added a suggestion to fix sorting.

I do have a question about the themes spec. Don't we want themes to override/use other themes and maybe also augment the graphql types of those child themes?

cc @gatsbyjs/themes-core

packages/gatsby/src/schema/index.js

michaellopez · 2019-08-30T07:19:05Z

@wardpeet

I do have a question about the themes spec. Don't we want themes to override/use other themes and maybe also augment the graphql types of those child themes?

We have this use case, currently we work around it via createNode + onCreateNode plus mediaType. But would definitely want that in core if possible.

wardpeet · 2019-08-30T07:25:04Z

Sweet! Good workaround 👍. My comment about parent/child augmentation isn't blocking to get this PR merged so most people will already be happy

stefanprobst · 2019-08-30T09:20:01Z

Don't we want themes to override/use other themes and maybe also augment the graphql types of those child themes?

#16529 did not really get lots of responses so i think this needs more discussion wrt to themes modifying types owned by other themes. this PR is a conservative start (following @KyleAMathews comment on slack)

ChristopherBiscardi · 2019-08-30T17:40:11Z

packages/gatsby/src/schema/schema.js

    plugin.name === `default-site-plugin` ||
-    plugin.name === typeOwner
+    (plugin.name.startsWith(`gatsby-theme-`) &&


This is a heavy handed suggestion but not a requirement, as detailed in #16529 (comment)

Also this code would need to be modified to work for scoped packages like @namespace/gatsby-theme-thing as well

wardpeet · 2019-09-02T06:17:02Z

Could you add some tests for non vs scoped packages?

stefanprobst · 2019-09-02T06:24:37Z

Sure, but let's first decide if we can actually rely on the naming convention?

…odify-types

freiksenet · 2019-09-19T08:10:03Z

@stefanprobst I feel that we should probably remove restriction on modifying types by other plugins. I'm not sure there is any other solution, we don't want a "theme-class" and "non-theme-class" plugins.

CC @ChristopherBiscardi @KyleAMathews

KyleAMathews · 2019-09-19T21:35:51Z

Perhaps we could do what we do for nodes? E.g. reserve a "fields" key on every type that other plugins can add additional fields to.

chenlijun99 · 2019-10-01T12:15:51Z

@KyleAMathews What I'd like to be possible is to override some types, instead of extending them.
E.g. override and make explicit, instead of inferred, the type of a frontmatter from markdown.

stefanprobst · 2019-10-11T10:07:07Z

@freiksenet

I feel that we should probably remove restriction on modifying types by other plugins. I'm not sure there is any other solution, we don't want a "theme-class" and "non-theme-class" plugins.

I'm not really clear why that is? To me it seems that the issue of "who is allowed to modify which types" is exactly a case where having some way to differentiate between themes and other-plugins would make sense (even if only by naming convention). Not sure I like dropping type ownership just to preserve the theme/plugin symmetry.

KyleAMathews · 2019-10-14T10:33:06Z

What we have to prevent is scenarios where s site is working and someone installs a plugin (or theme) and the site stops working. This is incredibly confusing for everyone.

Allowing plugins or themes to override types leads to exactly that sort of problem.

stefanprobst · 2019-10-16T10:24:45Z

ok, I'm going to close this as there doesn't seem to be consensus wrt to type modifications. if I understand correctly, @freiksenet's position is to remove restrictions for type modifications and allow them for all plugins, while @KyleAMathews's position is to not let plugins modify types at all.

let's clarify this in #16529

They don’t appear to be working even though themes are allowed to customise schemas. Reference: - gatsbyjs/gatsby#26864 - https://github.com/gatsbyjs/gatsby/issues/15544\ - gatsbyjs/gatsby#16928

* chore: bump gatsby, TS & React dependencies * Remove custom schema types They don’t appear to be working even though themes are allowed to customise schemas. Reference: - gatsbyjs/gatsby#26864 - https://github.com/gatsbyjs/gatsby/issues/15544\ - gatsbyjs/gatsby#16928 * Update peer deps for Gatsby

feat: Allow themes to modify types

c84cd0d

stefanprobst requested a review from a team as a code owner August 21, 2019 20:19

DSchau force-pushed the master branch from cf2ec88 to 561d33e Compare August 21, 2019 21:15

Fix array sort

270a02a

lannonbr added topic: GraphQL Related to Gatsby's GraphQL layer topic: themes labels Aug 22, 2019

wangyi7099 reviewed Aug 23, 2019

View reviewed changes

wardpeet previously approved these changes Aug 30, 2019

View reviewed changes

packages/gatsby/src/schema/index.js Show resolved Hide resolved

ChristopherBiscardi reviewed Aug 30, 2019

View reviewed changes

Handle scoped packages

b8bec09

stefanprobst dismissed wardpeet’s stale review via b8bec09 August 31, 2019 18:11

Fix dashes in scope

c30279d

Merge remote-tracking branch 'upstream/master' into allow-themes-to-m…

d75f420

…odify-types

stefanprobst mentioned this pull request Oct 11, 2019

fix(gatsby): Fix sort for Node 12 #18502

Merged

stefanprobst closed this Oct 16, 2019

stefanprobst deleted the allow-themes-to-modify-types branch October 16, 2019 10:24

ehowey mentioned this pull request Dec 19, 2019

Clarify GraphQL type ownership and permissions for themes #16529

Closed

russmatney mentioned this pull request Feb 24, 2020

fix: specify gql schema to prevent cover inferrence error Chronoblog/gatsby-theme-chronoblog#36

Closed

JohJonker mentioned this pull request May 25, 2020

Is there a workaround for customising schema from a Gatsby Theme? #24428

Closed

vladar mentioned this pull request Sep 11, 2020

feat(gatsby): merge GraphQL types defined by different plugins (with a warning) #26864

Merged

luqven mentioned this pull request Jul 2, 2021

Warning during build when using with gatsby-source-shopify imgix/gatsby#129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow themes to modify GraphQL types #16928

feat: Allow themes to modify GraphQL types #16928

stefanprobst commented Aug 21, 2019 •

edited

Loading

stefanprobst commented Aug 21, 2019

wangyi7099 Aug 23, 2019 •

edited

Loading

stefanprobst Aug 23, 2019

wangyi7099 Aug 23, 2019

stefanprobst Aug 30, 2019

wardpeet left a comment

michaellopez commented Aug 30, 2019

wardpeet commented Aug 30, 2019

stefanprobst commented Aug 30, 2019

ChristopherBiscardi Aug 30, 2019

ChristopherBiscardi Aug 30, 2019

wardpeet commented Sep 2, 2019

stefanprobst commented Sep 2, 2019

freiksenet commented Sep 19, 2019

KyleAMathews commented Sep 19, 2019

chenlijun99 commented Oct 1, 2019

stefanprobst commented Oct 11, 2019

KyleAMathews commented Oct 14, 2019

stefanprobst commented Oct 16, 2019

feat: Allow themes to modify GraphQL types #16928

feat: Allow themes to modify GraphQL types #16928

Conversation

stefanprobst commented Aug 21, 2019 • edited Loading

stefanprobst commented Aug 21, 2019

wangyi7099 Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

stefanprobst Aug 23, 2019

Choose a reason for hiding this comment

wangyi7099 Aug 23, 2019

Choose a reason for hiding this comment

stefanprobst Aug 30, 2019

Choose a reason for hiding this comment

wardpeet left a comment

Choose a reason for hiding this comment

michaellopez commented Aug 30, 2019

wardpeet commented Aug 30, 2019

stefanprobst commented Aug 30, 2019

ChristopherBiscardi Aug 30, 2019

Choose a reason for hiding this comment

ChristopherBiscardi Aug 30, 2019

Choose a reason for hiding this comment

wardpeet commented Sep 2, 2019

stefanprobst commented Sep 2, 2019

freiksenet commented Sep 19, 2019

KyleAMathews commented Sep 19, 2019

chenlijun99 commented Oct 1, 2019

stefanprobst commented Oct 11, 2019

KyleAMathews commented Oct 14, 2019

stefanprobst commented Oct 16, 2019

stefanprobst commented Aug 21, 2019 •

edited

Loading

wangyi7099 Aug 23, 2019 •

edited

Loading