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): migrate to schema customization API #32351

Conversation

axe312ger
Copy link
Collaborator

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.

Current status:

  • e2e tests do pass
  • unit tests for gatsby-node.js still need to be refactored. This might be a great time to restructure these huge fixture json files?
  • some @todo left

@axe312ger axe312ger added the topic: source-contentful Related to Gatsby's integration with Contentful label Jul 13, 2021
@axe312ger axe312ger requested a review from wardpeet July 13, 2021 13:57
@axe312ger axe312ger self-assigned this Jul 13, 2021
@axe312ger axe312ger added this to In progress in Contentful source plugin improvements via automation Jul 13, 2021
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 13, 2021
@@ -1,30 +1,19 @@
const path = require(`path`)
const isOnline = require(`is-online`)
// @todo import syntax!
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reminder to myself: time to finally get all files to es-module syntax

@@ -1,30 +1,19 @@
const path = require(`path`)
const isOnline = require(`is-online`)
// @todo import syntax!
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reminder to myself: time to finally get all files to es-module syntax


// If the cache has data, use it. Otherwise do a remote fetch anyways and prime the cache now.
// If present, do NOT contact contentful, skip the round trips entirely
if (forceCache) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This experimental flag adds some complexity to this code we could avoid.

I can't find any documentation to it as well

}
})

// @todo based on the sys metadata we should be able to differentiate new and updated entities
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should be low effort, also pretty low impact on the whole build time

}
})

// @todo based on the sys metadata we should be able to differentiate new and updated entities
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should be low effort, also pretty low impact on the whole build time

// Remove deleted entries & assets
reporter.verbose(`Removing deleted Contentful entries & assets`)

// @todo this should happen when sourcing?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMHO any interaction with actual nodes should happen in source-nodes. WDYT?

}
})

const creationActivity = reporter.activityTimer(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@todo consider including all the map creation and loops in the creation activity.


// A contentType can hold lots of entries which create nodes
// We wait until all nodes are created and processed until we handle the next one
// TODO add batching in gatsby-core
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wardpeet do we have any kind of new API that could help here? That comment is for sure very old


creationActivity.end()

// @todo add own activity!
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@todo do it

@axe312ger axe312ger added the type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change label Jul 13, 2021
@LekoArts LekoArts removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 28, 2021
@axe312ger axe312ger moved this from In progress to On Hold in Contentful source plugin improvements Aug 13, 2021
@wardpeet wardpeet force-pushed the refactor/contentful-asset-dowloading branch from e01c0f0 to e955764 Compare August 20, 2021 10:53
@axe312ger axe312ger marked this pull request as draft August 24, 2021 09:37
@wardpeet wardpeet force-pushed the refactor/contentful-asset-dowloading branch 2 times, most recently from 2ae4698 to 3f686a0 Compare September 1, 2021 10:11
@axe312ger axe312ger force-pushed the refactor/contentful-asset-dowloading branch from f441cb3 to 8bda979 Compare September 2, 2021 13:18
@axe312ger
Copy link
Collaborator Author

Closing in favour of #33207 and #33213

@axe312ger axe312ger closed this Sep 16, 2021
@axe312ger axe312ger deleted the refactor/contentful-use-schema-customization-api branch November 5, 2021 13:52
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 type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change
Development

Successfully merging this pull request may close these issues.

None yet

2 participants