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 Rich Text implementation in gatsby-source-contentful #25249

Merged
merged 17 commits into from
Nov 9, 2020

Conversation

axe312ger
Copy link
Collaborator

@axe312ger axe312ger commented Jun 24, 2020

This is a umbrella PR for the next major/breaking version of gatsby-source-contentful

This branch is published tagged as next. Try it out in your projects: npm i gatsby-source-contentful@next

Features

Breaking changes / migrations:

If you are not using the Rich Text Contentful field type there are no breaking changes.

Using Rich Text? Follow these migration steps:

Rich Text migration

  • 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 option as the new references field automatically resolves locales
  • To render Rich Text fields use the new renderRichText() function from gatsby-source-contentful/rich-text

Check this example code for a simple page template:

Example rendering code

import React from "react"
import { graphql, Link } from "gatsby"
import Img from "gatsby-image"
import * as propTypes from "prop-types"

// Import the new rendering and the render node definitions
import { renderRichText } from "gatsby-source-contentful/rich-text"
import { BLOCKS, INLINES } from "@contentful/rich-text-types"

import Layout from "../components/layout"

// Setting the rendering options. Same as:
// https://github.com/contentful/rich-text/tree/master/packages/rich-text-react-renderer
const options = {
  renderNode: {
    [INLINES.ENTRY_HYPERLINK]: ({
      data: {
        target: { slug, title },
      },
    }) => <Link to={slug}>{title}</Link>,
    [BLOCKS.EMBEDDED_ASSET]: node => <Img {...node.data.target} />,
  },
}

function PageTemplate({ data }) {
  const { title, description } = data.contentfulPage

  return (
    <Layout>
      <h1>{title}</h1>
      {/* Render the Rich Text field data: */}
      {description && renderRichText(description, options)}
    </Layout>
  )
}

PageTemplate.propTypes = {
  data: propTypes.object.isRequired,
}

export default PageTemplate

export const pageQuery = graphql`
  query pageQuery($id: String!) {
    contentfulPage(id: { eq: $id }) {
      title
      slug
      description {
        raw
        references {
          ... on ContentfulPage {
            # contentful_id is required to resolve the references
            contentful_id
            title
            slug
          }
          ... on ContentfulAsset {
            # contentful_id is required to resolve the references
            contentful_id
            fluid(maxWidth: 600) {
              ...GatsbyContentfulFluid_withWebp
            }
          }
        }
      }
    }
  }
`

@axe312ger axe312ger added effort: high topic: source-contentful Related to Gatsby's integration with Contentful labels Jun 24, 2020
@axe312ger axe312ger self-assigned this Jun 24, 2020
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 24, 2020
@wardpeet wardpeet added status: needs core review Currently awaiting review from Core team member and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jun 24, 2020
@axe312ger axe312ger force-pushed the contentful-next branch 2 times, most recently from 679fd35 to 9c627fc Compare June 30, 2020 12:26
@gatsby-cloud-staging
Copy link

gatsby-cloud-staging bot commented Jul 8, 2020

Gatsby Cloud Build Report

gatsby-master

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 1m

Performance

Lighthouse report

Metric Score
Performance 💚 98
Accessibility 🔶 87
Best Practices 💚 93
SEO 🔶 73

🔗 View full report

@benrobertsonio
Copy link
Contributor

@axe312ger @wardpeet are we ready to release a canary version of this?

@axe312ger
Copy link
Collaborator Author

@ben-rogerson yes, lets get it out into the wild and test it on a bigger scale :)

@RichardH9L
Copy link

@axe312ger It seems the type name of the references is always using the contenttype name instead of id

I'm using the setting useNameForId: false

image

@disintegrator
Copy link
Contributor

Hi @axe312ger, I know this is a little late in the game but one really useful feature to have in the plugin is the ability to customize locale fallback for assets. In Contentful, the fallback feature applies to all content (entries and assets). This almost never desirable and there doesn't seem to be any plans from them to make that configuration more granular. I noticed in the source plugin (based on 2.3.33) that there is a utility function used to fetch fields with a local fallback. I'm wondering if that can be fed some plugin config option.

In the following example, assets that don't have a file set for en-gb will use the file value for en-us.

module.exports = {
  plugins: [
    {
      resolve: "gatsby-source-contentful",
      options: {
        getFallbackLocale: (nodeType, fieldName, locale) => {
          if (
            nodeType === "ContentfulAsset" &&
            fieldName === "file" &&
            locale === "en-gb"
          ) {
            return "en-us";
          }
        },
      },
    },
  ],
};

The API is purposely abstracted to support not just assets but any Contentful node type. There are use cases to customise the fallback behaviour for other content types. Another example is that we store our translation phrases as entries in Contentful and if a phrase is defined in en-gb but not en-au then we can define a fallback for ContentfulTranslation node type from en-au to en-gb.

@axe312ger
Copy link
Collaborator Author

@disintegrator what you are asking for is a new, non-breaking feature. Can you please open a separate ticket for your feature request?

@axe312ger
Copy link
Collaborator Author

@hoenderdaal thanks, good catch. I'll check what going on and republish a fix!

@disintegrator
Copy link
Contributor

Thanks for the nudge. #26094

@axe312ger
Copy link
Collaborator Author

@hoenderdaal should be fixed when #26102 is reviewed :)

@pvdz
Copy link
Contributor

pvdz commented Oct 29, 2020

Current version published under gatsby-source-contentful@4.0.0-4.0.0-rc.0.20+15720d958 (or next)

Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

Let's move forward with this :)

@pvdz pvdz merged commit a256346 into master Nov 9, 2020
@delete-merged-branch delete-merged-branch bot deleted the contentful-next branch November 9, 2020 10:46
@pvdz
Copy link
Contributor

pvdz commented Nov 9, 2020

Published under gatsby-source-contentful@4.0.0-next.0 , scheduled for the next release on Thursday

@pvdz pvdz removed the status: needs core review Currently awaiting review from Core team member label Nov 11, 2020
@daydream05
Copy link
Contributor

daydream05 commented Nov 13, 2020

Super excited for the launch! Thank you to everyone's hardwork on this!

@pvdz
Copy link
Contributor

pvdz commented Nov 13, 2020

Published as gatsby-source-contentful@4.0.0

@grgcnnr
Copy link
Contributor

grgcnnr commented Nov 13, 2020 via email

@TjenWellens
Copy link

This took me too long to find, so I'm going to add my error messages here.
In the hopes that the next person having the same problem, will end up here, and see the workaround.

When you are following the migration guide:

export const pageQuery = graphql`
  query pageQuery($id: String!) {
    contentfulPage(id: { eq: $id }) {
      title
      slug
      description {
        raw
        references {
          ... on ContentfulAsset {
            # contentful_id is required to resolve the references
            contentful_id
            fluid(maxWidth: 600) {
              ...GatsbyContentfulFluid_withWebp
            }
          }
        }
      }
    }
  }
`

You get an error like

TypeError: Cannot read property '0' of undefined
getCurrentSrcData
node_modules/gatsby-image/index.js:134
|    return currentData[0];

getImageSrcKey
node_modules/gatsby-image/index.js:102

inImageCache
node_modules/gatsby-image/index.js:144

And you can resolve the error by adding __typename and contentful_id

 ... on ContentfulAsset {
# contentful_id is required to resolve the references
contentful_id
fluid(maxWidth: 600) {
  ...GatsbyContentfulFluid_withWebp
}
# __typename is ALSO required to resolve the references
__typename
}

TjenWellens added a commit to TjenWellens/gatsby that referenced this pull request Dec 29, 2020
Fail fast when rich-text and __typename not present
at least while gatsbyjs#28528 is not finished.
because figuring out what was wrong while migrating from v3 to v4, was a pain (gatsbyjs#25249 (comment))
@axe312ger
Copy link
Collaborator Author

@TjenWellens I adjusted the readme to include __typename in the example

@KishokanthJeganathan
Copy link

KishokanthJeganathan commented Feb 9, 2021

@axe312ger hey mate, the new plugin updates works like a charm on gatsby, but I cant yet figure out how to get the value for hyperlinks inside rich text.

Using the below in my renderNode, I can get access to the uri, but not the value

[INLINES.HYPERLINK]: ({ data }) => ( <a href={data.uri} > /*cant see value here*/ </a> )

@mhedengren
Copy link

@axe312ger hey mate, the new plugin updates works like a charm on gatsby, but I cant yet figure out how to get the value for hyperlinks inside rich text.

Using the below in my renderNode, I can get access to the uri, but not the value

[INLINES.HYPERLINK]: ({ data }) => ( <a href={data.uri} > /*cant see value here*/ </a> )

Hey, this is how I've done, maybe it helps you out.

`
const websiteUrl = "localhost:8000/"

[INLINES.HYPERLINK]: node => (
<a
href={node.data.uri}
target={${node.data.uri.startsWith(websiteUrl) ? "_self" : "_blank"}}
rel={${ node.data.uri.startsWith(websiteUrl) ? "" : "noopener noreferrer" }}
>
{node.content[0].value}

),`

@KishokanthJeganathan
Copy link

@mhedengren thanks mate, unfortunately, that used to work before they updated the package. Now it's different since json has now become a raw output. But I got the fix from a contentful dev-

https://stackoverflow.com/questions/66124543/how-to-render-hyperlinks-inside-contentful-rich-text/66125749#66125749

@mareksuscak
Copy link
Contributor

mareksuscak commented Feb 19, 2021

@axe312ger looks like people are still running into this issue because the changelog entry has not been updated and is still missing the __typename requirement, see:

https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-source-contentful/CHANGELOG.md#400-next0-2020-11-09

EDIT: submitted a PR: #29579

@axe312ger
Copy link
Collaborator Author

Changelog is updated now :)

@gusliedke
Copy link

Changelog is updated now :)

Hi @axe312ger I'm using "gatsby-source-contentful": "^5.1.2", . I don't think the documentation has been updated yet neither in github and contentful. It took me a while to figure out that contentful_id is required. It would be helpful for others if we can update it. Thanks!

@mareksuscak
Copy link
Contributor

mareksuscak commented Mar 31, 2021

@gusliedke it is documented in the changelog / migration guide, see:

CleanShot 2021-03-31 at 21 08 37

@orsi
Copy link
Contributor

orsi commented Oct 1, 2021

Yes, changelog and readme are adjusted

https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-source-contentful#contentful-rich-text

https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-source-contentful/CHANGELOG.md#400-next0-2020-11-09

Spent 30 minutes trying to find why my references were not coming through the renderer only to find this thread. contentful_id as a requirement is not stated in the documentation -- simply having an example query with it in there doesn't give me the impression it's required. There is no difference between title or slug fields being required if that's what it is supposed to indicate.

#33401

Screen Shot 2021-10-01 at 1 59 02 PM

@axe312ger
Copy link
Collaborator Author

Yes, changelog and readme are adjusted
https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-source-contentful#contentful-rich-text
https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-source-contentful/CHANGELOG.md#400-next0-2020-11-09

Spent 30 minutes trying to find why my references were not coming through the renderer only to find this thread. contentful_id as a requirement is not stated in the documentation -- simply having an example query with it in there doesn't give me the impression it's required. There is no difference between title or slug fields being required if that's what it is supposed to indicate.

#33401

Screen Shot 2021-10-01 at 1 59 02 PM

I know, thats unfortunate. I added that info everywhere I guess.

The next version won't suffer from this anymore: #31385

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: source-contentful Related to Gatsby's integration with Contentful
Projects
None yet
Development

Successfully merging this pull request may close these issues.