feat(gatsby-source-contentful): Resolve locales in fields of entries referenced by rich-text fields #14616

jessepinho · 2019-06-07T12:20:57Z

Description

When rich-text fields reference an entry or asset, the fields of the entry or asset don't have their locales resolved like the rest of the GraphQL response.

For example, consider the following GraphQL query:

{
  contentfulArticle(slug: { eq: "my-article" }, node_locale: { eq: "en-US" }) {
    title
    bodyRichText {
      json
    }
  }
}

The response will look like this:

{
  contentfulArticle: {
    title: 'My article',
    bodyRichText: {
      json: {
        content: [{ nodeType: 'embedded-entry-block', data: { target: { fields: { title: { 'en-US': 'My other article', 'de-DE': 'Meine andere Artikel' } } } } }],
      } 
    }
  }

Note that, while the title of the outer article has a resolved locale — that is, the title property is simply the title string in the requested locale — the title of the inner article does not have a resolved locale. Instead, the title property is an object containing en-US and de-DE properties. This makes for an inconsistent experience when working with the GraphQL response.

This PR fixes this issue by walking through the rich-text JSON tree and resolving locales of referenced entries. It also works recursively: if a referenced entry has its own reference to a third entry, it resolves the locales for the third (and fourth, and...) entry as well. It does the same for assets (although not recursively, since assets don't have reference fields).

Note that this PR includes a major version bump, as it includes a breaking change for the content of the rich-text JSON.

cc @Khaledgarbaya

Related Issues

Otherwise, it sounds like content types themselves are being created

BREAKING CHANGE: Fields in entries referenced by a rich-text field will now have their locales resolved. This means that, instead of accessing `entry.fields.fieldName.en`, you can just access the value at `entry.fields.fieldName`.

jessepinho · 2019-06-07T12:28:42Z

packages/gatsby-source-contentful/src/__tests__/normalize.js

@@ -55,7 +55,7 @@ describe(`Process contentful data`, () => {
    const createNodeId = jest.fn()
    createNodeId.mockReturnValue(`uuid-from-gatsby`)
    contentTypeItems.forEach((contentTypeItem, i) => {
-      normalize.createContentTypeNodes({
+      normalize.createNodesForContentType({


I felt that this name better explained what it's doing — i.e., it's not just creating the content type node itself (e.g., Article), but also the entries in the content type.

Yeah, thanks for that - I was confused quite few times by the name of the function in the past ;)

pieh · 2019-06-10T11:16:29Z

packages/gatsby-source-contentful/package.json

@@ -1,13 +1,14 @@
 {
  "name": "gatsby-source-contentful",
  "description": "Gatsby source plugin for building websites using the Contentful CMS as a data source",
-  "version": "2.0.67",
+  "version": "3.0.0",


We are bumping version as part of publishing process, so please revert version bump here. I marked PR with breaking change label to keep track of it

@pieh fixed — thanks!

pieh · 2019-06-10T11:44:39Z

Is this change compatible with https://github.com/contentful/rich-text/tree/master/packages/rich-text-react-renderer ?

jessepinho · 2019-06-10T12:13:57Z

@pieh I haven't specifically tested these two together, but I can't see why not. The rich-text renderer simply walks to the node tree and passes each node to a renderer, and I haven't changed the node tree at all. I've only changed the value of entry references that exist inside the node tree, which the rich-text renderer will take as-is and pass to the renderers that one provides in its config.

I feel like this is hard to explain by text 🙈 but does that make sense?

jessepinho · 2019-06-10T12:16:12Z

(Thanks for taking a look at this btw! Excited to get it merged!)

pieh · 2019-06-10T12:43:52Z

@pieh I haven't specifically tested these two together, but I can't see why not. The rich-text renderer simply walks to the node tree and passes each node to a renderer, and I haven't changed the node tree at all. I've only changed the value of entry references that exist inside the node tree, which the rich-text renderer will take as-is and pass to the renderers that one provides in its config.

I will test it out. I honestly didn't use package I linked in the previous comment, so I don't know details, but given that it's linked in our docs/plugin readme - it would be good to make sure it does.

I feel like this is hard to explain by text 🙈 but does that make sense?

I know the feel 🤗

jessepinho · 2019-06-10T12:46:47Z

Thank you! 🙏

…

On Mon 10. Jun 2019 at 14:44, Michal Piechowiak ***@***.***> wrote: @pieh <https://github.com/pieh> I haven't specifically tested these two together, but I can't see why not. The rich-text renderer simply walks to the node tree and passes each node to a renderer, and I haven't changed the *node* tree at all. I've only changed the value of entry references that exist inside the node tree, which the rich-text renderer will take as-is and pass to the renderers that one provides in its config. I will test it out. I honestly didn't use package I linked in the previous comment, so I don't know details, but given that it's linked in our docs/plugin readme - it would be good to make sure it does. I feel like this is hard to explain by text 🙈 but does that make sense? I know the feel 🤗 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#14616?email_source=notifications&email_token=AAIR2CCQXPT3WCSKYBSRIMDPZZECTA5CNFSM4HVWSFH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXJYU3I#issuecomment-500402797>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIR2CGJDYCFWET7KTSM6SDPZZECTANCNFSM4HVWSFHQ> .

pieh · 2019-06-10T17:55:16Z

I did test run and it still works with https://github.com/contentful/rich-text/tree/master/packages/rich-text-react-renderer with the change being that fields are localized from get-go (which is what we want).

I think it would be good to batch this PR and #12816 together for major version bump. I can make prerelease with this change so you could install gatsby-source-contentful@resolve-rich-text-locales (or something like that) before we get the other PR in shape to be merged

jessepinho · 2019-06-11T07:52:05Z

@pieh hm, is it not possible to simply do two major version bumps? My concern is just that, as this is a big repo, the chance of merge conflicts/etc. increases over time, and people won't be able to use this change in the meantime. Thoughts?

(If you absolutely want to wait on the major version bump, then the prerelease would help me a lot, thanks! However, does that mean I wouldn't get any new PRs that are merged into this repo until I switch back to @latest?)

jessepinho · 2019-06-11T08:14:53Z

Wait, just one quick thing: if we could just merge and prerelease this now, that'd be great and would unblock me for some issues I'm facing — regardless of whether we do a version bump now or later :)

pieh · 2019-06-11T12:41:03Z

@pieh hm, is it not possible to simply do two major version bumps?

We could do that, but we don't want to make two major version bumps within week (or so) of each other for single package. There is no technical problem with that, just it's more respectful to our users to batch those breaking changes so they don't have to go through potential migrations multiple times if they upgrade often.

Wait, just one quick thing: if we could just merge and prerelease this now, that'd be great and would unblock me for some issues I'm facing — regardless of whether we do a version bump now or later :)

This won't really work with how publishing with how lerna works (setup for our monorepo that helps with publishing to npm and package versioning). Anything merged into master will be published as latest.

jessepinho · 2019-06-11T12:46:20Z

@pieh hm, is it not possible to simply do two major version bumps?

We could do that, but we don't want to make two major version bumps within week (or so) of each other for single package. There is no technical problem with that, just it's more respectful to our users to batch those breaking changes so they don't have to go through potential migrations multiple times if they upgrade often.

OK, makes sense.

Wait, just one quick thing: if we could just merge and prerelease this now, that'd be great and would unblock me for some issues I'm facing — regardless of whether we do a version bump now or later :)

This won't really work with how publishing with how lerna works (setup for our monorepo that helps with publishing to npm and package versioning). Anything merged into master will be published as latest.

OK. I've made a fork of this that includes my fixes, so that I can buy some time and not rush this process. So I'll just upgrade once the major version bump is done. Thanks!

jessepinho · 2019-06-12T10:07:16Z

Note to @pieh: I've just pushed up another commit to handle array reference fields

jessepinho · 2019-06-12T10:39:07Z

@pieh I've just pushed another commit as well to fix an infinite loop issue

jessepinho · 2019-06-17T12:46:05Z

Hey @pieh, what's the status on this — waiting for the other PR to also merge?

jessepinho · 2019-06-25T16:26:51Z

Hey @pieh, just checking in — will this definitely not be merged until #12816 is?

pieh · 2019-06-25T17:40:58Z

#12816 is pretty much ready - it does need to be reviewed, so hopefully not much longer

…ich-text-references

pvdz

While this is stlil blocked for being backwards incompatible, how about we hide it under an option for now? That way we can merge this in and not worry about backwards compatibility.
Then the next step would be to think of a deprecation track, or perhaps mark it as something to include in "v3", what- or whenever that happens.

For this option we'd need the actual option added and the docs updated to describe the option.

pvdz · 2019-12-05T11:38:45Z

packages/gatsby-source-contentful/src/rich-text.js

+const getEntryContentType = (entry, contentTypes) =>
+  contentTypes.find(
+    contentType =>
+      entry.sys.contentType &&


Please move this check outside. entry.sys.contentType && contentTypes.find(...), and then cache the value of entry.sys.contentType.id because that won't change while searching here.

pvdz · 2019-12-05T11:43:02Z

packages/gatsby-source-contentful/src/rich-text.js

+  contentTypes,
+  getField,
+  defaultLocale,
+  resolvedEntryIDs = [],


This default is unnecessary; both callers of this function will explicitly pass on the array. Please remove it.

Suggested change

resolvedEntryIDs = [],

resolvedEntryIDs,

pvdz · 2019-12-05T11:46:29Z

packages/gatsby-source-contentful/src/rich-text.js

+      contentTypes,
+      getField,
+      defaultLocale,
+      resolvedEntryIDs: [...resolvedEntryIDs, field.sys.id],


I'm a little worried about the perf implications of creating a new array and spreading into it every time, instead of maintaining a single array and push/popping, or at least versus doing something like resolvedEntryIDs.concat(field.sys.id). Did you happen to check this? How frequent would this function be expected to be called at all? For a small site (100 pages)? For a large site (100.000 pages)?

jessepinho · 2020-01-13T15:08:49Z

@pvdz I was out on holidays for most of the last month — sorry for the delay! I've created a ticket on our end to address your comments. I really like the idea of wrapping it in a config flag so that it's backwards-compatible! Will let you know when I've made more progress on this — thanks!

jessepinho · 2020-02-21T13:19:53Z

Due to merge/rebase/conflict issues, I've superseded this PR with #21619. It hides this functionality behind a new config option, richText.resolveFieldLocales, and takes care of some of the optimization stuff @pvdz requested. Let's continue the convo over there.

jessepinho added 6 commits June 6, 2019 23:10

refactor: use a more descriptive function name

d54cf9b

Otherwise, it sounds like content types themselves are being created

feat: resolve locales for fields in referenced entries

6b74013

BREAKING CHANGE: Fields in entries referenced by a rich-text field will now have their locales resolved. This means that, instead of accessing `entry.fields.fieldName.en`, you can just access the value at `entry.fields.fieldName`.

refactor: use more sensible names

8745cae

refactor: remove unnecessary function

380886c

docs: add a brief explanation of the rich text normalizer

4a65761

feat: resolve locales for assets too

f6a5180

jessepinho commented Jun 7, 2019

View reviewed changes

pieh added the breaking change If implemented, this proposed work would break functionality for older versions of Gatsby label Jun 10, 2019

pieh reviewed Jun 10, 2019

View reviewed changes

jessepinho force-pushed the topics/localize-rich-text-references branch from f18c715 to f6a5180 Compare June 10, 2019 11:40

jessepinho requested a review from a team as a code owner June 10, 2019 11:40

pieh changed the title ~~[gatsby-source-contentful] Resolve locales in fields of entries referenced by rich-text fields~~ feat(gatsby-source-contentful): Resolve locales in fields of entries referenced by rich-text fields Jun 10, 2019

fix: resolve locales for array reference fields

80a23bb

jessepinho force-pushed the topics/localize-rich-text-references branch from e19842c to 80a23bb Compare June 12, 2019 10:06

fix: keep track of resolved entries to prevent infinite loops

b272ab1

pieh added 2 commits June 26, 2019 10:21

Merge branch 'master' into topics/localize-rich-text-references

2b85c63

Merge branch 'master' into topics/localize-rich-text-references

6d37011

sidharthachatterjee added the status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged label Aug 15, 2019

Merge remote-tracking branch 'upstream/master' into topics/localize-r…

0096646

…ich-text-references

pvdz reviewed Dec 5, 2019

View reviewed changes

jessepinho mentioned this pull request Feb 20, 2020

feat: add an option to resolve locales in entries referenced by rich text #21619

Merged

jessepinho closed this Feb 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gatsby-source-contentful): Resolve locales in fields of entries referenced by rich-text fields #14616

feat(gatsby-source-contentful): Resolve locales in fields of entries referenced by rich-text fields #14616

jessepinho commented Jun 7, 2019 •

edited

Loading

jessepinho Jun 7, 2019

pieh Jun 10, 2019

pieh Jun 10, 2019

jessepinho Jun 10, 2019

pieh commented Jun 10, 2019

jessepinho commented Jun 10, 2019

jessepinho commented Jun 10, 2019

pieh commented Jun 10, 2019

jessepinho commented Jun 10, 2019 via email

pieh commented Jun 10, 2019

jessepinho commented Jun 11, 2019

jessepinho commented Jun 11, 2019

pieh commented Jun 11, 2019

jessepinho commented Jun 11, 2019

jessepinho commented Jun 12, 2019

jessepinho commented Jun 12, 2019

jessepinho commented Jun 17, 2019

jessepinho commented Jun 25, 2019

pieh commented Jun 25, 2019

pvdz left a comment

pvdz Dec 5, 2019

pvdz Dec 5, 2019

pvdz Dec 5, 2019

jessepinho commented Jan 13, 2020

jessepinho commented Feb 21, 2020

feat(gatsby-source-contentful): Resolve locales in fields of entries referenced by rich-text fields #14616

feat(gatsby-source-contentful): Resolve locales in fields of entries referenced by rich-text fields #14616

Conversation

jessepinho commented Jun 7, 2019 • edited Loading

Description

Related Issues

jessepinho Jun 7, 2019

Choose a reason for hiding this comment

pieh Jun 10, 2019

Choose a reason for hiding this comment

pieh Jun 10, 2019

Choose a reason for hiding this comment

jessepinho Jun 10, 2019

Choose a reason for hiding this comment

pieh commented Jun 10, 2019

jessepinho commented Jun 10, 2019

jessepinho commented Jun 10, 2019

pieh commented Jun 10, 2019

jessepinho commented Jun 10, 2019 via email

pieh commented Jun 10, 2019

jessepinho commented Jun 11, 2019

jessepinho commented Jun 11, 2019

pieh commented Jun 11, 2019

jessepinho commented Jun 11, 2019

jessepinho commented Jun 12, 2019

jessepinho commented Jun 12, 2019

jessepinho commented Jun 17, 2019

jessepinho commented Jun 25, 2019

pieh commented Jun 25, 2019

pvdz left a comment

Choose a reason for hiding this comment

pvdz Dec 5, 2019

Choose a reason for hiding this comment

pvdz Dec 5, 2019

Choose a reason for hiding this comment

pvdz Dec 5, 2019

Choose a reason for hiding this comment

jessepinho commented Jan 13, 2020

jessepinho commented Feb 21, 2020

jessepinho commented Jun 7, 2019 •

edited

Loading