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

Refactor Contentful Rich Text Field integration #24905

Merged
merged 4 commits into from
Jul 8, 2020

Conversation

axe312ger
Copy link
Collaborator

@axe312ger axe312ger commented Jun 10, 2020

Complex data models in combination with Contentful rich text can cause issues. The code can become slow, the worst case will crash the build process.

This is a follow up to #15084 and tries to fix #24221

More detailed description here: #24221 (comment)

  • Instead of storing resolved Rich Text Document Data in GraphQL, we will store the raw unresolved document in GraphQL. This will already let our build process pass. But now we only have the raw Contentful Link available when rendering rich text.
  • Next, we add a subfield to the rich text graphql field references for querying data of references entities via GraphQL.
  • For rendering, we are going to introduce GraphQL biggest change. The current best practice is to use the official Rich Text Renderer directly with the GraphQL field result. We expose a new function from the source plugin, which will wrap documentToReactComponents and takes care of resolving the references based on our custom GraphQL Query data.

Code demo:

import React from "react"

import Layout from "../components/layout"
import SEO from "../components/seo"

/* Import the render function: */
import { renderRichText } from "gatsby-source-contentful/rich-text"

const IndexPage = ({ data }) => {
  return (
    <Layout>
      <SEO title="Home" />
      {data.allContentfulPage.nodes.map(({title, richTextField}) => (
        <div>
          <h1>{title}</h1>
          {/* Render the Rich Text field: */}
          {richTextField && renderRichText(richTextField, {
            // Same options as:
            // https://github.com/contentful/rich-text/tree/master/packages/rich-text-react-renderer
          })}
        </div>
      ))}
    </Layout>
  )
}

export const pageQuery = graphql`
  query {
    allContentfulPage(limit: 1) {
      nodes {
        title
        richTextField {
          raw
          # Query the fields you need on your references:
          references {
            contentful_id # contentful_id is required
            ... on ContentfulPage {
              title
            }
            ... on ContentfulProduct {
              name
            }
          }
        }
      }
    }
  }
`

export default IndexPage

@axe312ger axe312ger requested a review from a team as a code owner June 10, 2020 12:15
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 10, 2020
@axe312ger axe312ger self-assigned this Jun 10, 2020
@axe312ger axe312ger added impact: high topic: source-contentful Related to Gatsby's integration with Contentful type: bug An issue or pull request relating to a bug in Gatsby labels Jun 10, 2020
@axe312ger
Copy link
Collaborator Author

I have a working reference subfield. But it only works for a single reference node type. https://github.com/gatsbyjs/gatsby/pull/24905/files#diff-dc42475a9f0ed45d6e1bf554ec9958a2R556

Next step is to allow all possible Contentful node types as reference.

@KyleAMathews
Copy link
Contributor

Will this require a major release of gatsby-source-contentful?

@axe312ger
Copy link
Collaborator Author

@KyleAMathews this won't be breaking when not using rich text. Rich text users would require to extend their query if they use linked entities and change 2-3 lines of code per template file.

@danabrit danabrit removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 11, 2020
@DSchau
Copy link
Contributor

DSchau commented Jun 17, 2020

@axe312ger

this won't be breaking when not using rich text. Rich text users would require to extend their query if they use linked entities and change 2-3 lines of code per template file.

this is breaking then, right? If a user has to change their code (e.g. if they're using RichText) then we should designate this release as such (e.g. bump to a new major version).

Nicely done here!

@axe312ger
Copy link
Collaborator Author

@DSchau yeah, even when its not likely that the page was rendering before the fix, we should mark it as breaking change

@axe312ger axe312ger force-pushed the fix/contentful-rich-text-references branch from 49ac1e4 to cce9c6b Compare June 17, 2020 14:37
@axe312ger
Copy link
Collaborator Author

Thanks to @vladar we now have an interface for all Contentful content types.

Will now work on the rendering part!

@axe312ger
Copy link
Collaborator Author

Some features we will get with this PR:

  • Properly request rich text referenced assets & entries via graphql
  • This means you can finally render assets via fluid and fixed in a performant non-hacky way
  • Neat side effect: You will be able to query multiple Contentful content types at once. For example: If you have a mixed list of news and products, you can now query them in one Query. Saving time and code for stitching the two arrays together into one list.

@@ -547,13 +546,44 @@ exports.extendNodeType = ({ type, store }) => {
return {
nodeType: {
type: GraphQLString,
deprecationReason: `This field is deprecated, please use 'json' instead.`,
deprecationReason: `This field is deprecated, please use 'raw' instead. @todo add link to migration steps.`,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to myself:

Make sure to add migrations steps into the warnings!

@axe312ger
Copy link
Collaborator Author

I pushed the first version of the rendering function.

Usage is pretty simple:

  1. import render function
  2. Add fields you need from the references to your GraphQL query
  3. Call the render function with the result of the query
import React from "react"

import Layout from "../components/layout"
import SEO from "../components/seo"

/* Import the render function: */
import { renderRichText } from "gatsby-source-contentful/rich-text"

const IndexPage = ({ data }) => {
  return (
    <Layout>
      <SEO title="Home" />
      {data.allContentfulPage.nodes.map(({title, richTextField}) => (
        <div>
          <h1>{title}</h1>
          {/* Render the Rich Text field: */}
          {richTextField && renderRichText(richTextField, {
            // Same options as:
            // https://github.com/contentful/rich-text/tree/master/packages/rich-text-react-renderer
          })}
        </div>
      ))}
    </Layout>
  )
}

export const pageQuery = graphql`
  query {
    allContentfulPage(limit: 1) {
      nodes {
        title
        richTextField {
          raw
          # Query the fields you need on your references:
          references {
            contentful_id
            ... on ContentfulPage {
              title
            }
            ... on ContentfulProduct {
              name
            }
          }
        }
      }
    }
  }
`

export default IndexPage

A few issues:

  • Resolving the entries ist not as trivial, since we prefix the Contenful ids sometimes (!) with a c. We need to change the fixId function or just get rid of it? It is probably just a relict of old times: [1.0] [gatsby-source-contentful] Left-pad IDs starting with a number to make GraphQL happy #1276
  • Still no support for assets as they have to be handled in a different way. Will probably split the references field up in {references { Entry: { ...}, Asset: { ... }}}
  • There is a huge chunk of code for the resolveFieldLocales option. I do think we can remove all of it as the new code makes this feature obsolete. (Opinions welcome, I never used this feature!)
  • The code is not locale aware yet. If your entry is available in multiple languages, you probably might get a random one of these for rendering a Rich Text Field.

Ideas and opinions very welcome. I will continue next week.

Have a great weekend,
Benedikt

@axe312ger axe312ger added the breaking change If implemented, this proposed work would break functionality for older versions of Gatsby label Jun 23, 2020
@axe312ger axe312ger force-pushed the fix/source-contentful-link-resolution branch 2 times, most recently from 1409cf9 to 135b32e Compare June 23, 2020 12:28
@axe312ger axe312ger force-pushed the fix/contentful-rich-text-references branch from e432498 to 0a5d8f9 Compare June 24, 2020 09:10
@axe312ger axe312ger force-pushed the contentful-next branch 2 times, most recently from 679fd35 to 9c627fc Compare June 30, 2020 12:26
@axe312ger axe312ger force-pushed the fix/contentful-rich-text-references branch 3 times, most recently from 5f8d1dd to b7eee19 Compare June 30, 2020 14:18
@axe312ger axe312ger added the status: needs core review Currently awaiting review from Core team member label Jul 1, 2020
@axe312ger axe312ger changed the title WIP - Refactor Contentful Rich Text Field integration Refactor Contentful Rich Text Field integration Jul 1, 2020
Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Overall looks good to me 👍 but added a couple of minor suggestions inline.

Copy link

@DanweDE DanweDE left a comment

Choose a reason for hiding this comment

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

I don't know much about Gatsby but usage of RT seems fine @axe312ger 👍

packages/gatsby-source-contentful/src/rich-text.js Outdated Show resolved Hide resolved
packages/gatsby-source-contentful/src/rich-text.js Outdated Show resolved Hide resolved
packages/gatsby-source-contentful/src/rich-text.js Outdated Show resolved Hide resolved
const dummyResponse = {
items: [
{
sys: { type: `Entry` },
Copy link

Choose a reason for hiding this comment

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

Looking at https://github.com/contentful/contentful-resolve-response it doesn't seem necessary to include the sys unless I am missing something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It uses the type to create lookup keys and avoid id collisions

https://github.com/contentful/contentful-resolve-response/blob/master/index.js#L20

@axe312ger axe312ger force-pushed the fix/contentful-rich-text-references branch 2 times, most recently from 5a31d85 to 09ede3c Compare July 8, 2020 11:01
me4502 and others added 4 commits July 8, 2020 16:15
…es (#23514)

* fix(gatsby-source-contentful): fixed id collision in contentful entries
…o support circular references within Rich Text fields
* Allow circular references
* Improve performance and reduce RAM footprint
* Query referenced entries and assets via GraphQL

fixes #24221

BREAKING CHANGE:

* Entities references in Rich Text fields are no more automatically resolved
* Use the `raw` subfield instead of `json`
* Use GraphQL to define your referenced data with the new `references` field
* Removes the `resolveFieldLocales` as the new `references` field automatically resolves locales
* To render Rich Text fields unse the new `renderRichText()` function from `gatsby-source-contentful/rich-text`
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 status: needs core review Currently awaiting review from Core team member topic: source-contentful Related to Gatsby's integration with Contentful type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants