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-contentful): provide gql type definitions using content model introspection and schema-customization #12816

Closed
wants to merge 36 commits into from

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Mar 24, 2019

This is work in progress. It works, but there are few things that need to be cleaned up before merging this.

This works currently (at least with using-contentful example) - you can swap currently used Contentful space configuration in using-contentful/gatsby-config.js to one that is currently commented out. This space doesn't have any content but all queries still work and you can build a site (even if it does not have any content apart from things that are hardcoded in react components).

Potential blockers (discussion points)

  • parent/children - this is big one. Right now transformers rely on actually having parent nodes. There are some notes in code (search for // Long string (markdown))
  • there is currently small issue in Contentful API with single reference fields when you setup validation to restrict possible content types. API currently doesn't pass that information back to the client (it does for multiple references fields). So right now we create unions for those fields (which can be seen by change in one of queries in using-contentful example)

I might have forgotten some things - I tried to add pretty detailed comments beside the code.

Breaking changes:

This also introduce number of breaking changes:

  • JSON fields return GraphQLJSON type:
    • JSON field was inferred before, but we can't replicate that with free-form json content. I also generally think returning it as JSON type and not object makes much more sense
    • RichText: json field inside richText field is preserved (to maintain somewhat compatibility) but all other fields are gone (one that was already deprecated) and others that were inferred (they were pretty useless anyway, as it's really hard to query all the fields from structured content like that)
  • Union type name for reference fields can be different - it's produced from used type names and is using same method as one that inferred union by Gatsby, but because it's not relying on data - there might be extra types there. This can break fragments

TO-DO:

  • adjust documentation - probably as last step, as things can still change
  • revert temporary changes in using-contentful example's gatsby-config

/cc @Khaledgarbaya @sbezludny @freiksenet @stefanprobst

@pieh pieh added status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged breaking change If implemented, this proposed work would break functionality for older versions of Gatsby labels Mar 24, 2019
@pieh pieh requested a review from a team as a code owner March 24, 2019 22:02
@pieh
Copy link
Contributor Author

pieh commented Mar 24, 2019

@Khaledgarbaya
Copy link
Contributor

Hi @pieh,
This looks really Great

there is currently small issue in Contentful API with single reference fields when you setup validation to restrict possible content types. API currently doesn't pass that information back to the client (it does for multiple references fields). So right now we create unions for those fields (which can be seen by change in one of queries in using-contentful example)

I think we can live with the Union type for now as it's the easiest approach for now. I don't think having the validation for single ref fields will happen anytime soon.

For the complex example I can definetly help with testing on complex examples

@sbezludny
Copy link
Contributor

Hi @pieh

I think straight returning JSON of structured content make sense, as right now you need to query for...

Own Contentful's GraphQL API exposes json field on rich text types. It is done to separate content from the link resolution (links field) which for this plugin is handled by contentful.js SDK.

Other than that, I don't see reasons for keeping this field.

@pieh
Copy link
Contributor Author

pieh commented Mar 25, 2019

Own Contentful's GraphQL API exposes json field on rich text types. It is done to separate content from the link resolution (links field) which for this plugin is handled by contentful.js SDK.

Oh, interesting - I might revisit this to be more inline with how Contentful GraphqQL have RichText fields set up (once we deal with changed that we need to make in gatsby package). I see value in having schema generated by gatsby-source-contentful and Yours to be as similar as possible (i.e. easier for users to migrate queries from one source to another).

@Khaledgarbaya
Copy link
Contributor

Hey @pieh Anything we can help with the unblock this?

@Khaledgarbaya Khaledgarbaya added the status: awaiting author response Additional information has been requested from the author label Apr 24, 2019
@pieh
Copy link
Contributor Author

pieh commented Apr 24, 2019

This is partly blocked by #13028 - after that we will be able to get rid of parts where I was importing gatsby internals (which is not safe):

// Ideally we would have nice access to it - I also potentially missed something
// obvious and I don't need to import from gatsby internals (yikes #1)
const { link } = require(`gatsby/dist/schema/resolvers`)
// This probably will be handled by automatically adding fieldArgs/resolver
// to Date fields - for now importing directly from gatsby internals (yikes #2)
const { dateResolver } = require(`gatsby/dist/schema/types/date`)

I will also need to dial back unneeded back-references changes and address rich text types changes.

Was there any update on validation field for single references in Contentful API (didn't check in quite a bit)?

@Khaledgarbaya
Copy link
Contributor

Khaledgarbaya commented Apr 24, 2019

Was there any update on validation field for single references in Contentful API (didn't check in quite a bit)?

I don't think it will happen anytime soon so we can live with a union to all content types

@pieh pieh added status: WIP and removed status: awaiting author response Additional information has been requested from the author labels May 20, 2019
@m-allanson m-allanson added this to In progress in OSS Roadmap via automation May 20, 2019
@pieh pieh removed the status: WIP label May 21, 2019
@pieh
Copy link
Contributor Author

pieh commented Sep 16, 2019

While we are kind of blocked here with merging this one, maybe you could try checking if creating temporarily placeholder context to create schema snapshot with https://www.gatsbyjs.org/packages/gatsby-plugin-schema-snapshot/, and then removing placeholders and relying on generated snapshot would work?

@mmellado
Copy link

mmellado commented Oct 1, 2019

While we are kind of blocked here with merging this one, maybe you could try checking if creating temporarily placeholder context to create schema snapshot with https://www.gatsbyjs.org/packages/gatsby-plugin-schema-snapshot/, and then removing placeholders and relying on generated snapshot would work?

I can confirm this works. I created a Contentful environment in which I ensured to have at least one entry will all fields for each content type as well as a reference to all possible types of components in every multi relationship field. Generated the schema using the https://www.gatsbyjs.org/packages/gatsby-plugin-schema-snapshot/ plugin, then unpublished all entries for some content types. Build passed ignoring the content types with no entries as they were still generated in the GraphQL schema (they simply return no results). Null checks are still necessary but this at least allows me to have queries in gatsby-node.js to generate pages for content types which may not necessarily have entries all the time. Thanks!

@kitos
Copy link
Contributor

kitos commented Oct 10, 2019

I spend more time on this yesterday - mostly trying this in practice in I right now have doubts about decision to convert reference fields that allow single type to create 1-type unions.

While it makes sense to do this, given that at any time you can edit reference field to allow accepting more types - and gatsby changing from GraphQLObject to GraphQLUnion would break queries. But just looking at what queries with references allowing single type look like - it's really messy:

@pieh what about adding plugin option - alwaysCastReferenceFieldsToUnions?

@bsgreenb
Copy link

Question: will this do anything to allow making Required-> Non_Null? I'm using graphql simply for a static build and would like to verify my Required fields are present.

I know there's issue of Preview environments.. I'm fine one-off handling these on the frontend. I'm building for production and would like to be able to require things with typescript generated from graphql.

@pieh
Copy link
Contributor Author

pieh commented Nov 14, 2019

Question: will this do anything to allow making Required-> Non_Null? I'm using graphql simply for a static build and would like to verify my Required fields are present.

Current implementation does make Contentful required field as NonNull Graphql fields and I do want to keep it that way for production builds / when using "production" data from Contentful.

I'm not sure how exactly to make it to continue work with Contentful's Preview API yet (as queries would just fail if any of required field is not there). Maybe this is fine for them to not work/update results until all required fields are filled, but the DX when leaving it like that will be awful. One way we could make it work is if we use mock field values for required fields when using preview API (but not in production mode). So empty text field would be something like <No content in required field "[fieldName]"> (scalar fields are pretty "easy" to mock, reference fields would be harder, but possible).

I guess question is - do we want types to change (probably not) or are we fine with mocked data for required fields to workaround the issue.

@bsgreenb
Copy link

@pieh What if it was configurable on a per-environment basis what Required meant? So for the server using preview environment, you could build with those fields being nullable based on a config param?

Would totally be fine with using default fallbacks for preview environment.

Comment on lines 7 to 11
const digest = str =>
crypto
.createHash(`md5`)
.update(str)
.digest(`hex`)
Copy link
Contributor

Choose a reason for hiding this comment

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

new code style suggestion from:

#8805 Issue: feat: update gatsby plugins to use new createContentDigest helper

Suggested change
const digest = str =>
crypto
.createHash(`md5`)
.update(str)
.digest(`hex`)
const digest = str => createContentDigest(str)

and add to imports:

import { createContentDigest } from "gatsby-core-utils"

or better replace all usages of digest with createContentDigest

@m-allanson m-allanson moved this from Prioritized to In progress in OSS Roadmap Jan 7, 2020
@pvdz pvdz marked this pull request as draft April 9, 2020 08:03
@pvdz pvdz removed the status: WIP label Apr 9, 2020
@pvdz
Copy link
Contributor

pvdz commented Apr 9, 2020

(I've converted this PR into a Draft PR. Although it's been a year since the start, is any progress expected at all?)

@kitos
Copy link
Contributor

kitos commented Apr 21, 2020

(I've converted this PR into a Draft PR. Although it's been a year since the start, is any progress expected at all?)

it is expected 🙂
I believe it should improve build performance

@bsgreenb
Copy link

bsgreenb commented Apr 28, 2020

I want to follow up on this. The Maybe<> that wraps everything pulled by the graphql code generator requires me to overwrite all my types, and gives me no info about fields that are actually optional or required.

Here are my type overrides, which are a chore to maintain and somewhat eliminate the benefits of typescript https://gist.github.com/bsgreenb/05202d74c7ef079970b536fad0fc5c88

@pieh was concerned about preview environments, but why dont you just disallow previewing content that doesn't have required fields? The only fields I require on a post are a title/body/slug. I want the build to break if it doesn't have those, and an editor can easily enter them.

This issue has been around over a year, and the entire time that I've been working on my project. I'm sure there's a workaround that let's us actually sync up TypeScript and Contentful!

@axe312ger axe312ger added this to In progress in Contentful source plugin improvements via automation May 28, 2020
@danabrit danabrit added topic: source-contentful Related to Gatsby's integration with Contentful and removed effort: med labels May 28, 2020
@axe312ger axe312ger moved this from In progress to To do in Contentful source plugin improvements Jun 23, 2020
@LekoArts
Copy link
Contributor

@axe312ger @pieh Just to make sure: Is this change also needed with the new source plugin for Contentful and/or will this be part of other changes already?

@axe312ger
Copy link
Collaborator

@LekoArts still needed and one of the highest ranking issues for us after getting rich text run with complex content models!

@rshackleton
Copy link

rshackleton commented Jan 7, 2021

I spend more time on this yesterday - mostly trying this in practice in I right now have doubts about decision to convert reference fields that allow single type to create 1-type unions.
While it makes sense to do this, given that at any time you can edit reference field to allow accepting more types - and gatsby changing from GraphQLObject to GraphQLUnion would break queries. But just looking at what queries with references allowing single type look like - it's really messy:

@pieh what about adding plugin option - alwaysCastReferenceFieldsToUnions?

Rather than using a union, could we use the base type ContentfulReference or ContentfulEntry as this supports using inline fragments to query fields for all potential types? This wouldn't suffer from issues with the type name changing.

We used a similar approach for the Kentico Kontent plugin by using the kontent_item type for all relationships.

This approach also works when patching the schema locally:

function customiseHomeModel() {
  const typeDefs = `
    type ContentfulHome implements Node {
      metadata: ContentfulReference @link(by: "id", from: "metadata___NODE")
      sections: [ContentfulReference] @link(by: "id", from: "sections___NODE")
    }
  `;

  return typeDefs;
}

In the GraphQL query you can then use an inline fragment to pull any data you need:

query MyQuery {
  allContentfulHome {
    nodes {
      sections {
        contentful_id
        id
        ... on ContentfulArticle {
          id
          date
          slug
          title
        }
      }
    }
  }
}

@axe312ger
Copy link
Collaborator

Closing this PR in favour of #30855.

Feel free to comment and test the upcoming major version.

@axe312ger axe312ger closed this Apr 27, 2021
@axe312ger axe312ger deleted the contentful-schema branch April 29, 2021 10:04
@axe312ger axe312ger moved this from In progress to Done in Contentful source plugin improvements Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change If implemented, this proposed work would break functionality for older versions of Gatsby topic: GraphQL Related to Gatsby's GraphQL layer topic: source-contentful Related to Gatsby's integration with Contentful
Projects
OSS Roadmap
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet