feat(gatsby-source-contentful): move types into createSchemaCustomization #33207

axe312ger · 2021-09-16T10:01:33Z

Replacement of #32351

cleanup: The plugin filled up sourceNodes with a bunch of things. This PR cleans this up, moves large chunks of the code in separate files and their correct GatsbyJS API hooks.
GATSBY_CONTENTFUL_EXPERIMENTAL_FORCE_CACHE got removed
plugin options: forceFullSync removed as it wasn't supported for a while and added enableTags to validation
e2e: update the snapshots to align with the just merged 33236 which enforces https for images
e2e: update schema.gql for later comparision with new schema generation
~~cleanup: enable typescript check and move to import syntax~~ (moved to chore(gatsby-source-contentful): clean up code base and introduce es-module syntax #33213)
~~feature: improve logging by separating logs for new and updated entities~~ (moved to chore(gatsby-source-contentful): clean up code base and introduce es-module syntax #33213)

packages/gatsby-source-contentful/src/on-pre-bootstrap.js

DanielSLew · 2021-09-16T13:22:28Z

packages/gatsby-source-contentful/src/on-pre-bootstrap.js

+const fetchData = require(`./fetch`)
+const { createPluginConfig } = require(`./plugin-options`)
+
+export async function onPreBootstrap(


@wardpeet we would want to make sure with v4 that this function is onPluginInit. Otherwise the workers with source nodes and createSchemaCustomization will have the wrong data, correct?

It does a lot of tricky things that I don't understand why we're doing it in onPreBootstrap or would even do it in onPluginInit.

Why are we fetching data here?

Yes, if you don't call this code, you will stay with the old Contentful data

I understand but why can't we put this all in sourcenodes? This makes it super complex, I don't even understand what's happening here 😬

@wardpeet I need the content types to be available in createSchemaCustomization

The next "major" #31385 will require even more data from Contentful to available when creating the schema.

It might be possible to split up the huge fetch function into two functions:

createSchemaCustomization will load content types
sourceNodes will load content types + all of the rest

Loading data twice is bad, thats why I originally moved all of this into bootstrap. Could work around this with the Gatsby cache, but only if I can rely on the call order of createSchemaCustomization && sourceNodes

WDYT?

Why can't we call contentTypes separate from data? Doesn't contentful supports that? Relying on cache will be problematic when the process crashes - you don't really know where we left off.

We can, it is supported. I just wonder If i can avoid calling contentTypes twice.

Will try it now, let u know where I end up :)

GATSBY_CONTENTFUL_EXPERIMENTAL_FORCE_CACHE is going to make it tricky, as it assumes we have all the data in one place which will not be true anymore.

I might store the content type information in a separate file 🤔

I got a variant working in e2e and manual tests. Unit tests are green except the huge snapshot based tests. Will fix these after lunch, then we should be able to do another review round :)

axe312ger · 2021-09-17T12:14:12Z

@wardpeet @DanielSLew

I split up the fetching and processing of content types from the rest of the data we get from Contentful.

Snapshots: these always shared the same content types within all file. I moved all content types data into their own fixture
fetch.js: now split up into two functions, one for content types, the other one for the rest
onPreBootstrap: will now only create the cache directory for assets
createSchemaCustomization:
1. fetches content types
2. processes them
3. ~~stores these in Gatsby cache for sourceNodes (is this save?)~~ not anymore
4. creates the (incomplete aka inheriting) schema
sourceNodes:
1. gets ~~content types and~~ a copy of the last build content from Gatsby cache
2. exits early and uses existing data when GATSBY_CONTENTFUL_OFFLINE === true
3. fetches new content from Contentful
4. fetches latest content types from Contentful
5. merges fetched data with data from cache
6. stores merged build data in cache
7. resolves links
8. touches existing nodes
9. deletes deleted nodes
10. create new nodes
11. downloads assets to FS if requested
the flag GATSBY_CONTENTFUL_EXPERIMENTAL_FORCE_CACHE got removed as it did not work since tags got enabled and removal was approved by @benrobertsonio

packages/gatsby-source-contentful/src/fetch.js

DanielSLew · 2021-09-17T14:09:25Z

Thank you for the detailed walk through of the changes, that really helps to follow what's happening.

stores these in Gatsby cache for sourceNodes (is this save?)

I think the following comment from @wardpeet sums this up

Relying on cache will be problematic when the process crashes - you don't really know where we left off.

I'm still a little fuzzy as to what this experimental force cache does and what the purpose of it is? Is this a feature that people actually use?

benrobertsonio · 2021-09-17T14:26:03Z

The experimental force cache was a way to bypass the sourcing step. It was primarily used internally when working with customers with really large datasets. It was always undocumented / experimental. I think the customer success team was the primary user of that flag, and we aren't actively using it anymore. I'm fine with removing it.

axe312ger · 2021-09-17T14:44:25Z

Relying on cache will be problematic when the process crashes - you don't really know where we left off.

Alright, then I'll request the content types twice from Contentful. As this is only a minimal amount of data compared to the actual content, we should not add to much overhead.

axe312ger · 2021-09-17T15:05:22Z

I updated the code to request fresh content type data for every node sourcing and schema customization and updated my comment above to reflect the new flow.

KyleAMathews · 2021-09-25T18:25:42Z

Why refetch content types for every sourcing? Is that needed? I assume this would slow down inc builds?

axe312ger · 2021-09-26T08:16:09Z

Why refetch content types for every sourcing? Is that needed? I assume this would slow down inc builds?

That was my first approach. But Daniel and Ward say storing the content types in cache is not save? #33207 (comment)

I can still revert/remove 59af410 to use Gatsby cache for storing the content types

wardpeet

Added questions and comments. There are some possible race conditions with cache & node store

wardpeet · 2021-10-07T13:59:04Z

packages/gatsby-source-contentful/src/create-schema-customization.js

+    reporter,
+  })
+
+  createTypes(`


@vladar is it faster to only run createTypes once vs many times or are things the same?

@wardpeet @vladar as far as I can see the whole codebase calls createTypes with multiple types at ones.

So I went for that way, more clean, tests pass locally :)

packages/gatsby-source-contentful/src/create-schema-customization.js

packages/gatsby-source-contentful/src/source-nodes.js

packages/gatsby-source-contentful/src/fetch.js

… with tags

…e sourcing

…ization and node sourcing

…operly add enableTags

…a customization and node sourcing" This reverts commit 59af410.

Co-authored-by: Ward Peeters <ward@coding-tech.com>

wardpeet

If tests are green, let's get this in! 🚢 Thanks a ton @axe312ger

…tion (gatsbyjs#33207) Co-authored-by: Ward Peeters <ward@coding-tech.com>

…tion (#33207) Co-authored-by: Ward Peeters <ward@coding-tech.com>

@axe312ger

This bit me today when I wanted to use it. Apparently `forceFullSync` was removed years ago. PR's: gatsbyjs#33238, gatsbyjs#33207 @axe312ger mentioned he'd remove it in the new version, but since that's not live yet I'm proposing it here.

@axe312ger

… from readme (#38787) * Remove `forceFullSync` option from readme This bit me today when I wanted to use it. Apparently `forceFullSync` was removed years ago. PR's: #33238, #33207 @axe312ger mentioned he'd remove it in the new version, but since that's not live yet I'm proposing it here. * remove option from create-gatsby as well --------- Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 16, 2021

axe312ger commented Sep 16, 2021

View reviewed changes

packages/gatsby-source-contentful/src/on-pre-bootstrap.js Outdated Show resolved Hide resolved

axe312ger requested a review from DanielSLew September 16, 2021 10:10

DanielSLew reviewed Sep 16, 2021

View reviewed changes

This was referenced Sep 16, 2021

chore(gatsby-source-contentful): clean up code base and introduce es-module syntax #33213

Merged

refactor(contentful): migrate to schema customization API #32351

Closed

axe312ger commented Sep 17, 2021

View reviewed changes

packages/gatsby-source-contentful/src/fetch.js Outdated Show resolved Hide resolved

axe312ger mentioned this pull request Sep 17, 2021

feat(gatsby-source-contentful): pqr compat #33179

Closed

LekoArts added topic: source-contentful Related to Gatsby's integration with Contentful and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Sep 20, 2021

axe312ger force-pushed the refactor/prepare-contentful-for-gatsby-v4 branch from a607dd1 to 80f3df8 Compare September 24, 2021 08:24

axe312ger mentioned this pull request Sep 25, 2021

contentful: plugin options - remove unused forceFullSync and properly add enableTags #33238

Closed

axe312ger force-pushed the refactor/prepare-contentful-for-gatsby-v4 branch 2 times, most recently from 573bc2b to 97cacdf Compare September 25, 2021 12:14

wardpeet reviewed Oct 7, 2021

View reviewed changes

veryspry requested a review from wardpeet October 12, 2021 16:39

axe312ger added 7 commits October 12, 2021 22:13

refactor(contentful): move code to createSchemaCustomization

a16e21f

move all node manipulating code into sourceNodes

12daa97

move asset cache dir creation to pre bootstrap

53dc7f8

fix(contentful): GATSBY_CONTENTFUL_EXPERIMENTAL_FORCE_CACHE now works…

2ed4927

… with tags

fix GATSBY_CONTENTFUL_OFFLINE

e5e685e

fix(contentful): add enableTags to plugin options

ca28e3a

fix(contentful): ensure enabled tags do not break warm builds

cbceb73

axe312ger and others added 20 commits October 12, 2021 22:13

fix unit tests

d6a012f

refactor(contentful): fetch content types for schema and data for nod…

e5e7526

…e sourcing

refactor(contentful): simplify fetch code and add more comments

845e19f

refactor(contentful): remove GATSBY_CONTENTFUL_EXPERIMENTAL_FORCE_CACHE

2b6f64f

refactor(contentful): get latest content types on every schema custom…

8ed2235

…ization and node sourcing

docs(contentful): plugin options - remove unused forceFullSync and pr…

00e2c24

…operly add enableTags

test(contentful): update e2e test snapshots for https

6bd317e

chore(contentful): update e2e test schema for comparision

8eed1da

chore: add dontInfer to interfaces and align code style of type creation

ea2e840

Revert "refactor(contentful): get latest content types on every schem…

bbebda5

…a customization and node sourcing" This reverts commit 59af410.

store sync token in plugin status

053eb57

Co-authored-by: Ward Peeters <ward@coding-tech.com>

lint

e6639b8

fix tests for pluginStatus change

56f026f

ensure syncProgress does tick and finish

40eb0df

properly create interfaces

978ac43

ensure syncProgress reports properly when no data was available for sync

1c394ef

fix unit tests

7978708

style: call createTypes once

7ea2a54

use cache data in PQR workers

f3b91b0

fix review exports

4f22ccf

wardpeet force-pushed the refactor/prepare-contentful-for-gatsby-v4 branch from f49eaf9 to 4f22ccf Compare October 12, 2021 20:14

wardpeet changed the title ~~Prepare gatsby-source-contentful for Gatsby v4~~ feat(gatsby-source-contentful): move types into createSchemaCustomization Oct 12, 2021

wardpeet approved these changes Oct 12, 2021

View reviewed changes

wardpeet merged commit d01a373 into master Oct 13, 2021

wardpeet deleted the refactor/prepare-contentful-for-gatsby-v4 branch October 13, 2021 07:10

wardpeet added a commit to herecydev/gatsby that referenced this pull request Oct 29, 2021

feat(gatsby-source-contentful): move types into createSchemaCustomiza…

40864fa

…tion (gatsbyjs#33207) Co-authored-by: Ward Peeters <ward@coding-tech.com>

axe312ger added a commit that referenced this pull request Nov 9, 2021

feat(gatsby-source-contentful): move types into createSchemaCustomiza…

c71c2b4

…tion (#33207) Co-authored-by: Ward Peeters <ward@coding-tech.com>

bartveneman mentioned this pull request Dec 28, 2023

fix(gatsby-source-contentful): remove outdated forceFullSync option from readme #38787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gatsby-source-contentful): move types into createSchemaCustomization #33207

feat(gatsby-source-contentful): move types into createSchemaCustomization #33207

axe312ger commented Sep 16, 2021 •

edited

Loading

DanielSLew Sep 16, 2021

wardpeet Sep 16, 2021

axe312ger Sep 16, 2021

wardpeet Sep 16, 2021 •

edited

Loading

axe312ger Sep 16, 2021

axe312ger Sep 16, 2021 •

edited

Loading

wardpeet Sep 16, 2021

axe312ger Sep 17, 2021

axe312ger Sep 17, 2021

axe312ger Sep 17, 2021

axe312ger commented Sep 17, 2021 •

edited

Loading

DanielSLew commented Sep 17, 2021

benrobertsonio commented Sep 17, 2021

axe312ger commented Sep 17, 2021

axe312ger commented Sep 17, 2021

KyleAMathews commented Sep 25, 2021

axe312ger commented Sep 26, 2021

wardpeet left a comment

wardpeet Oct 7, 2021

axe312ger Oct 10, 2021

wardpeet left a comment

feat(gatsby-source-contentful): move types into createSchemaCustomization #33207

feat(gatsby-source-contentful): move types into createSchemaCustomization #33207

Conversation

axe312ger commented Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axe312ger Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axe312ger commented Sep 17, 2021 • edited Loading

DanielSLew commented Sep 17, 2021

benrobertsonio commented Sep 17, 2021

axe312ger commented Sep 17, 2021

axe312ger commented Sep 17, 2021

KyleAMathews commented Sep 25, 2021

axe312ger commented Sep 26, 2021

wardpeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet left a comment

Choose a reason for hiding this comment

axe312ger commented Sep 16, 2021 •

edited

Loading

wardpeet Sep 16, 2021 •

edited

Loading

axe312ger Sep 16, 2021 •

edited

Loading

axe312ger commented Sep 17, 2021 •

edited

Loading