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

Clarify GraphQL type ownership and permissions for themes #16529

Closed
stefanprobst opened this issue Aug 11, 2019 · 14 comments · Fixed by #26864
Closed

Clarify GraphQL type ownership and permissions for themes #16529

stefanprobst opened this issue Aug 11, 2019 · 14 comments · Fixed by #26864
Labels
topic: GraphQL Related to Gatsby's GraphQL layer

Comments

@stefanprobst
Copy link
Contributor

When building the GraphQL schema, we keep track of which types are created by which plugins. This information is used to prevent plugins from modifying types they do not own.

The exception to this is default-site-plugin, i.e. a user's gatsby-node.js, which is always allowed to modify types. When a type has been modified by a user, it is then owned by the user.

We should clarify, how this should work with themes. Currently, to the schema builder a theme appears like any other plugin and is therefore not allowed to modify types owned by other plugins. For example, a theme's gatsby-node.js would not be allowed to modify MdxFrontmatter, which is owned by gatbsy-plugin-mdx.

Options:

  • treat a theme like any other plugin, which means modifying types from a theme's gatsby-node is not allowed
  • treat a theme's gatsby-node like a top-level default-site-plugin
  • define a special set of permissions for a theme's gatsby-node

the last two options would both require a way to differentiate a theme from other plugins -- by gatsby-theme-* naming convention or something else.

personally, I opt for option 2+naming convention

/cc @freiksenet @ChristopherBiscardi

Related #15544

@lannonbr lannonbr added topic: themes topic: GraphQL Related to Gatsby's GraphQL layer labels Aug 12, 2019
@stefanprobst
Copy link
Contributor Author

Looks like naming themes with gatsby-theme-* is now a requirement? (#16620) This makes things easier.

@sidharthachatterjee
Copy link
Contributor

@stefanprobst Was always a requirement 😉

Just got codified in #16620

@ChristopherBiscardi
Copy link
Contributor

On a code level for defining theme features like shadowing and composition, the gatsby-theme-* prefix was never a hard requirement and still isn't. So the gatsby-theme-* naming convention is technically speaking a really heavy-handed suggestion.

I believe the only way in which that is "special" is because of babel-related processing code. You can easily work around this with the es6-compile plugin though and we shouldn't add any more features that require this naming convention since that would distance further "themes" from "plugins" which are currently the same thing. gatsby-plugin-theme-ui takes advantage of shadowing, for example.

@stefanprobst
Copy link
Contributor Author

I realize it's not enforced in code, but my understanding of #16620 and @sidharthachatterjee's comment was that it is a requirement by convention now. If not, we should not say so in the docs.

Personally I do think we need something to distinguish themes from other plugins if we want to give them permissions that non-theme plugins don't have, like modifying types they don't own.

@pieh
Copy link
Contributor

pieh commented Aug 30, 2019

The requirement for package names to contain gatsby (so it's actually less strict than `gatsby-theme-*) comes from code that handles query extraction and webpack config (to apply webpack rule that's rather heavy) - https://github.com/gatsbyjs/gatsby/search?q=gatsby-dependents&unscoped_q=gatsby-dependents.

This is not strictly about theme APIs (shadowing and gatsby-config composition).

This was supposed to enable not only this for themes but also components / components library that aren't themes/plugins to ship with queries (mostly static queries) and allow users to import them without having to add them to plugin list.

It probably make sense to merge list of packages that contain gatsby (case of packaged reusable components) with packages provided in plugins list (themes) for query extraction / webpack rule test, to lift restriction on theme package names, as our heuristic proved to be problematic, confusing and hardly discoverable.

@wangyi7099
Copy link
Contributor

@pieh Why not find theme in gatsby-config.js and add it to all dependencies list manually ?

@wardpeet
Copy link
Contributor

wardpeet commented Sep 2, 2019

@wangyi7099 what do you mean with find themes in gatsby-config? A theme = plugin and is part of the plugins list so it's hard to know exactly what a "theme" is.

@wangyi7099
Copy link
Contributor

@wardpeet Hi I mean we can judge the plugin whether it is theme or not by finding all related gatsby-config.js 's `__experimentalThemes keys.

@pieh
Copy link
Contributor

pieh commented Sep 2, 2019

@wardpeet Hi I mean we can judge the plugin whether it is theme or not by finding all related gatsby-config.js 's `__experimentalThemes keys.

__experimentalThemes is soft deprecated - we now recommend to use plugins array (as themes are really plugins too). We still might use both __experimentalThemes and plugins to construct final list of things to extract queries from and apply heavier webpack rules, but it does require some research (I don't if we had reason not to do this in the first place or it was just oversight)

@gatsbot
Copy link

gatsbot bot commented Sep 23, 2019

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.

If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!

As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

@gatsbot gatsbot bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Sep 23, 2019
@gatsbot
Copy link

gatsbot bot commented Oct 4, 2019

Hey again!

It’s been 30 days since anything happened on this issue, so our friendly neighborhood robot (that’s me!) is going to close it.

Please keep in mind that I’m only a robot, so if I’ve closed this issue in error, I’m HUMAN_EMOTION_SORRY. Please feel free to reopen this issue or create a new one if you need anything else.

As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks again for being part of the Gatsby community!

@gatsbot gatsbot bot closed this as completed Oct 4, 2019
@stefanprobst
Copy link
Contributor Author

I don't think this has been clarified.

@stefanprobst stefanprobst reopened this Oct 11, 2019
@freiksenet freiksenet added not stale and removed stale? Issue that may be closed soon due to the original author not responding any more. labels Oct 17, 2019
@freiksenet
Copy link
Contributor

OK, so decision here is whether we want to have special kind of plugins that are themes or are all plugins created equal.

It seems that @ChristopherBiscardi wants to not have any special theme treatment. However this conflicts with our restrictions of what plugins can do with different types.

In a way, we might be just blocked because we don't have plugins do the types for themselves, instead of relying on inference. It could be that if plugins actually all provided types, then it would be okay without having an option to modify other types.

We can also enable some type modification, eg when there is no type defined in owner plugin. Main idea we have for themes is that for "data abstraction types" there would be a child type defined that implements the abstraction interface anyway, so there is no reason to do type modification.

@ehowey
Copy link
Contributor

ehowey commented Dec 19, 2019

I would just second that this is still a concern when building themes. The use case I have run into recently was trying to make sure the siteMetaData object was more robust and if a field was deleted or not present then the site continued to function as normal.

For example I have a field keywords: in my siteMetaData and I was trying to define this as a graphql schema type in the themes gatsby-node.js so that if a user deleted this field or did not want it then the site would continue to function without failing on the query when building. I hope that makes sense. From looking at the discussion here and in #16928 it looks pretty complicated, so I can understand why this would not be a priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: GraphQL Related to Gatsby's GraphQL layer
Projects
None yet
10 participants