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

perf(gatsby-source-contentful): experimental flag to skip id normalization #25473

Merged
merged 1 commit into from
Jul 6, 2020

Conversation

pvdz
Copy link
Contributor

@pvdz pvdz commented Jul 2, 2020

This adds an experimental flag, EXPERIMENTAL_CONTENTFUL_SKIP_NORMALIZE_IDS, to skip the normalization step when sourcing from Contentful.

This step would take the sys.id and force it to a string and not start with a number. This step is quite expensive at scale, easily taking 5 minutes or more, because it has to traverse every property on every object of the content retrieved by Contentful.

While this step may have been necessary a long time ago, everybody agrees that today it's not required.

We may concretely remove the step in the future, but since that's a breaking change (people can query the contentful_id field in graphql, or access the contentful_id property directly inside a node) that's going to take a little longer to be published. This flag serves as a workaround until that time.

@pvdz pvdz requested a review from a team as a code owner July 2, 2020 12:11
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 2, 2020
@pvdz pvdz added topic: source-contentful Related to Gatsby's integration with Contentful topic: performance Related to runtime & build performance topic: scaling builds and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jul 2, 2020
@pvdz pvdz force-pushed the contentful-skip-id-norm branch from 873e6e4 to bd2b713 Compare July 2, 2020 12:52
@axe312ger axe312ger self-requested a review July 2, 2020 16:13
@pvdz pvdz force-pushed the contentful-skip-id-norm branch from bd2b713 to cf66d69 Compare July 6, 2020 08:51
@gatsby-cloud-staging
Copy link

Your pull request can be previewed in Gatsby Cloud: https://build-1abee51d-ddc6-4fea-a1a4-2d9242ccd9bf.staging-previews.gtsb.io

Copy link
Collaborator

@axe312ger axe312ger left a comment

Choose a reason for hiding this comment

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

Has high potential to fix issues and due to the flag we won't break peoples projects. Lets go :)

@xavivars
Copy link
Contributor

xavivars commented Aug 19, 2020

@pvdz: Will there be any way to access the contentful_id after this got merged?

I think I originally misunderstood the change. Am I right that contentful_id now is "exactly" the same Id that the contentful entry has, instead of being a modified version of it?

@pvdz
Copy link
Contributor Author

pvdz commented Aug 19, 2020

Correct me if I'm wrong but that should be the .id now, since it's no longer normalized? Before that was "backed up" in contentful_id.

@axe312ger
Copy link
Collaborator

id will be a custom id, build from the content type and the id of the entry. (As entrys of different types can share the same id)

The contentful_id will now contain the real id from contentful, without any alteration.

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

Successfully merging this pull request may close these issues.

3 participants