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

fix(gatsby-source-contentful): fix missing data with rich text fields #15084

Merged
merged 13 commits into from
Jun 24, 2020

Conversation

Khaledgarbaya
Copy link
Contributor

@Khaledgarbaya Khaledgarbaya commented Jun 24, 2019

Description

This PRs is an attempt to fix missing data on content changes at contentful.
the gatsby-source-plugin is using the Contentful SDK which is calling the sync API behind the scene for the first sync the SDK will have all the data available to it so it can perform the link resolution however in the subsequent sync the API will return only the changed entries which is not enough data for the SDK to do the link resolution.

Solution

By saving the previous sync data and using it later to do link resolution we make sure that all links are resolved

Related Issues

TODO

  • Add tests existing tests should do

@Khaledgarbaya Khaledgarbaya requested a review from a team as a code owner June 24, 2019 14:36
@DSchau
Copy link
Contributor

DSchau commented Jun 24, 2019

Awesome @Khaledgarbaya! I'll pull this down and see if I'm able to fix any issues.

Thanks for prioritizing this--it's been a sore spot for some users so super excited to get this fix in as soon as possible!

@FLoppix
Copy link

FLoppix commented Jun 25, 2019

Thanks for the fast help and response ! @DSchau and @Khaledgarbaya. Super pumped when this is fixed and let me know when I can test it or you need help / reproduction cases!

@Khaledgarbaya
Copy link
Contributor Author

Khaledgarbaya commented Jun 26, 2019

Sorry closed the PR by mistake.
@DSchau and @FLoppix I think you can test with this change and let me know.
I might have some more time tomorrow to write some tests

@pieh
Copy link
Contributor

pieh commented Jun 26, 2019

I've published gatsby-source-contentful@rich-text-resolve to make it easier to test

@FLoppix
Copy link

FLoppix commented Jun 26, 2019

I grabbed the package from @pieh and it still giving me not all the updated values.
I described it here in more detail #14918

@Khaledgarbaya
Copy link
Contributor Author

Hey @FLoppix Thanks for trying this out. @DSchau any chance I can get the space id and api key for the reproduction space?

@Khaledgarbaya Khaledgarbaya force-pushed the fix/source-contentful-link-resolution branch 2 times, most recently from a6ce40e to 2e24997 Compare June 28, 2019 14:04
@Khaledgarbaya
Copy link
Contributor Author

Hey @FLoppix and @DSchau I think my last change did fix the issue would you mind give it another try.

@DSchau
Copy link
Contributor

DSchau commented Jun 28, 2019

@Khaledgarbaya sorry - missed this! I’ll DM you on Twitter in 5.

@FLoppix
Copy link

FLoppix commented Jul 1, 2019

@Khaledgarbaya Your fix is working for me :)
Thank you so much ! This will help me a lot

@DSchau DSchau self-assigned this Jul 1, 2019
@Khaledgarbaya Khaledgarbaya force-pushed the fix/source-contentful-link-resolution branch from 2e24997 to a55a690 Compare July 2, 2019 07:21
@Khaledgarbaya Khaledgarbaya changed the title WIP: source-contentful Fix Link resolution on the next sync source-contentful Fix Link resolution on the next sync Jul 2, 2019
@odorisioe
Copy link

Hi @Khaledgarbaya, 2.1.2-rich-text-resolve.18 solves the hot reloading issue for me (thanks for that!), so I will wait for the merge.

However, I would like to check about an issue with entries included on rich text that could be introduced with this fix. I am not sure if it exists before cause I was not able to see rich text data because the error.

If I update an entry included on a rich text, the hot reload update the entry but does not update the json where it is included.

Example:
I created a content type Page with a rich text called "body" where I can embed VideoBlock entries.
If I update an VideoBlock entry, the reload works and updates the new values on Gatsby but the json still have old values. Only when I update a field on Page (like "title") the json is generated again.

@Khaledgarbaya
Copy link
Contributor Author

Hi @odorisioe,
Thanks for the test would you mind making a small video about the issue so I can look at it.

@odorisioe
Copy link

Hi @Khaledgarbaya I will try to create a video soon but these are the steps to reproduce the error

Steps

  1. (On Contentful) Create two content types
    Page
    . title (field type: string)
    . content (field type: rich text - allow to embed VideoBlock entries)
    VideoBlock
    . url (type: string)
    . caption (type: string)

  2. (On Contentful) Create a Page entry and add a title

  3. (On Contentful) Embed a VideoBlock on the field content of the Page created on step 2.

  4. (On Gatsby) Run "gatsby develop" with ENABLE_GATSBY_REFRESH_ENDPOINT=true

  5. (On Gatsby) Go to /__graphql and query by id the Page and the VideoBlock. The values are sync with Contentful.

# Get created page by id - include the rich text as json
query PageByIdQuery($id: String!) {
  contentfulPage(id: { eq: $id }) {
    id
    content {
      json
    }
  }
}

# Get video block by id (the one embedded on the rich text)
query VideoBlockByIdQuery($id: String!) {
  contentfulVideoBlock(id: { eq: $id }) {
    id
    url
    caption
  }
}
  1. (On Contentful) Update values on the VideoBlock - url and/or caption fields

  2. (On Gatsbty) Do a POST request to /__refresh and the VideoBlock entry will be updated locally.

Refreshing source data
Starting to fetch data from Contentful
Fetching default locale
default locale is : en-US
contentTypes fetched 12
Updated entries  1
Deleted entries  0
Updated assets  0
Deleted assets  0
Fetch Contentful data: 3245.120ms
  1. (On Gatsby) Repeat the same queries as the step 5 and compare the values of the VideoBlock.

Result

VideoBlockByIdQuery includes the updated values added on step 6.
But the rich-text json on PageByIdQuery includes the VideoBlock with the previous values

Do POST request to /__refresh do not regenerate the rich-text json.
Unless I make a change on the Page entry like edit the title. Then on next hot reloading, the json will be generated with the right values

@Khaledgarbaya
Copy link
Contributor Author

Hey @pieh @DSchau is it possible to release a new version of this to npm so people can play around with it more.

@DSchau
Copy link
Contributor

DSchau commented Jul 23, 2019

@Khaledgarbaya for sure. I'll do that today!

@DSchau
Copy link
Contributor

DSchau commented Jul 23, 2019

Successfully published:

  • gatsby-source-contentful@2.1.5-rich-text-resolve.19+787dd4b9f

Install with

npm i gatsby-source-contentful@rich-text-resolve

@DSchau DSchau changed the title source-contentful Fix Link resolution on the next sync fix(gatsby-source-contentful): fix missing data with rich text fields Jul 23, 2019
Ishaan28malik
Ishaan28malik previously approved these changes Jul 27, 2019
@axe312ger axe312ger force-pushed the fix/source-contentful-link-resolution branch from 1409cf9 to 135b32e Compare June 23, 2020 12:28
Copy link

@phoebeschmidt phoebeschmidt left a comment

Choose a reason for hiding this comment

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

usage of Cf SDK looks 👍 to me! very excited this is being fixed :)

Copy link
Contributor Author

@Khaledgarbaya Khaledgarbaya left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for picking this up

@axe312ger
Copy link
Collaborator

Thanks @phoebeschmidt & @Khaledgarbaya

So lets continue for #24905

@axe312ger axe312ger merged commit d43005d into contentful-next Jun 24, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix/source-contentful-link-resolution branch June 24, 2020 08:45
pvdz pushed a commit that referenced this pull request Oct 5, 2020
…e performance (#27244)

* fix(gatsby-source-contentful): fixed id collision in contentful entries (#23514)

* fix(gatsby-source-contentful): fixed id collision in contentful entries

* fix(gatsby-source-contentful): properly resolve subsequent sync calls   (#15084)

* fix(gatsby-source-contentful): fixed id collision in contentful entries (#23514)

* fix(gatsby-source-contentful): fixed id collision in contentful entries

* improve logging and clean up code (#26238)

* improve comment about contentful-resolve-response usage

* test(unit): remove cross test data pollution

* further improve loggin by replacing console.time with GatsbyReporter

* improve code style

* missing file

* remove logging verbosity

* adjust mocked reporter for tests to pass

* remove unused deep-map dependency

* fix: use content type id by default to create reference gql types (#26102)

* temporary console.time calls for performance measurements

* perf(gatsby-source-contentful): use a map and less loops to merge old and latest sync API response

* log time Contentful API processing  and GraphQL node creation

* upgrade dependencies and use faster node 10 versions

* remove duplicate logging

* allow multiple Contentful sources

fixes #26875

* refactor(imports): fix dependency structure and make sharp optional

fixes #23904

* align tests to match new multi source cache pattern

Co-authored-by: Matthew Miller <mnmiller1@me.com>
Co-authored-by: Khaled Garbaya <khaled@contentful.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs core review Currently awaiting review from Core team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.