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): fix API, execute deprecations, improve performance #27244

Merged
merged 13 commits into from
Oct 5, 2020

Conversation

axe312ger
Copy link
Collaborator

@axe312ger axe312ger commented Oct 2, 2020

This extracts all non Rich Text related fixes from #25249

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 2, 2020
@axe312ger axe312ger added status: needs core review Currently awaiting review from Core team member 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 Oct 2, 2020
@axe312ger axe312ger requested a review from pvdz October 2, 2020 12:18
@pvdz pvdz removed the status: needs core review Currently awaiting review from Core team member label Oct 2, 2020
@gatsbyjs gatsbyjs deleted a comment from gatsbot bot Oct 2, 2020
me4502 and others added 13 commits October 2, 2020 16:57
…es (gatsbyjs#23514)

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

* fix(gatsby-source-contentful): fixed id collision in contentful entries
* 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
@axe312ger axe312ger force-pushed the contentful-next-pre-rich-text branch from 794cc1e to 958ba8a Compare October 2, 2020 15:23
Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

lgtm, let's get this merged.

The data structure in buildForeignReferenceMap should be able to leverage Map and Set, but that looks to be out of scope for this PR.

@@ -189,37 +263,61 @@ exports.sourceNodes = async (
useNameForId: pluginConfig.get(`useNameForId`),
})

reporter.verbose(`Resolving Contentful references`)

const newOrUpdatedEntries = []
Copy link
Contributor

Choose a reason for hiding this comment

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

This ought to be a Set but we can fix that in a followup PR.

Comment on lines +426 to +430
return mId(
space.sys.id,
v.sys.id,
v.sys.linkType || v.sys.type
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This sort of thing happens a lot. I think some can be abstracted a little better (but not all).

@pvdz pvdz changed the title gatsby-source-contentful - fix API, execute deprecations, improve performance perf(gatsby-source-contentful): fix API, execute deprecations, improve performance Oct 5, 2020
@pvdz pvdz merged commit 7bf2bdb into gatsbyjs:master Oct 5, 2020
@pvdz
Copy link
Contributor

pvdz commented Oct 5, 2020

(Merged per agreement with @axe312ger)

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants