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.

Sign up for GitHub

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

Contentful next cleanup #26238

Merged
merged 8 commits into from
Aug 5, 2020
Merged

Contentful next cleanup #26238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dd16ac6
improve comment about contentful-resolve-response usage
axe312ger Aug 5, 2020
c15004a
test(unit): remove cross test data pollution
axe312ger Aug 5, 2020
f1e8b01
further improve loggin by replacing console.time with GatsbyReporter
axe312ger Aug 5, 2020
a8c05b3
improve code style
axe312ger Aug 5, 2020
178faf8
missing file
axe312ger Aug 5, 2020
8c99b79
remove logging verbosity
axe312ger Aug 5, 2020
e6b4d5c
adjust mocked reporter for tests to pass
axe312ger Aug 5, 2020
ad8ac07
remove unused deep-map dependency
axe312ger Aug 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/gatsby-source-contentful/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
"chalk": "^2.4.2",
"contentful": "^7.14.6",
"contentful-resolve-response": "^1.1.4",
"deep-map": "^1.5.0",
"fs-extra": "^8.1.0",
"gatsby-core-utils": "^1.3.14",
"gatsby-plugin-sharp": "^2.6.24",
Expand Down
3,546 changes: 1,784 additions & 1,762 deletions packages/gatsby-source-contentful/src/__fixtures__/starter-blog-data.js
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions packages/gatsby-source-contentful/src/__tests__/fetch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,11 @@ beforeAll(() => {

const reporter = {
info: jest.fn(),
verbose: jest.fn(),
panic: jest.fn(),
activityTimer: () => {
return { start: jest.fn(), end: jest.fn() }
},
}

beforeEach(() => {
Expand Down
89 changes: 45 additions & 44 deletions packages/gatsby-source-contentful/src/__tests__/gatsby-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ jest.mock(`gatsby-core-utils`, () => {
}
})

const _ = require(`lodash`)
const gatsbyNode = require(`../gatsby-node`)
const fetch = require(`../fetch`)
const normalize = require(`../normalize`)
Expand All @@ -30,6 +29,10 @@ describe(`gatsby-node`, () => {
const getCache = jest.fn()
const reporter = {
info: jest.fn(),
verbose: jest.fn(),
activityTimer: () => {
return { start: jest.fn(), end: jest.fn() }
},
}
const createNodeId = jest.fn(value => value)
let currentNodeMap
Expand Down Expand Up @@ -265,9 +268,7 @@ describe(`gatsby-node`, () => {
it(`should create nodes from initial payload`, async () => {
cache.get.mockClear()
cache.set.mockClear()
fetch.mockImplementationOnce(() =>
_.cloneDeep(startersBlogFixture.initialSync)
)
fetch.mockImplementationOnce(startersBlogFixture.initialSync)
const locales = [`en-US`, `nl`]

await gatsbyNode.sourceNodes({
Expand All @@ -282,14 +283,14 @@ describe(`gatsby-node`, () => {
schema,
})

testIfContentTypesExists(startersBlogFixture.initialSync.contentTypeItems)
testIfContentTypesExists(startersBlogFixture.initialSync().contentTypeItems)
testIfEntriesExists(
startersBlogFixture.initialSync.currentSyncData.entries,
startersBlogFixture.initialSync.contentTypeItems,
startersBlogFixture.initialSync().currentSyncData.entries,
startersBlogFixture.initialSync().contentTypeItems,
locales
)
testIfAssetsExistsAndMatch(
startersBlogFixture.initialSync.currentSyncData.assets,
startersBlogFixture.initialSync().currentSyncData.assets,
locales
)

Expand All @@ -301,11 +302,11 @@ describe(`gatsby-node`, () => {
// Stores sync token and raw/unparsed data to the cache
expect(cache.set).toHaveBeenCalledWith(
`syncToken`,
startersBlogFixture.initialSync.currentSyncData.nextSyncToken
startersBlogFixture.initialSync().currentSyncData.nextSyncToken
)
expect(cache.set).toHaveBeenCalledWith(
`previousSyncData`,
startersBlogFixture.initialSync.currentSyncData
startersBlogFixture.initialSync().currentSyncData
)
expect(cache.set.mock.calls.length).toBe(2)
})
Expand All @@ -314,11 +315,11 @@ describe(`gatsby-node`, () => {
const locales = [`en-US`, `nl`]

fetch
.mockReturnValueOnce(_.cloneDeep(startersBlogFixture.initialSync))
.mockReturnValueOnce(_.cloneDeep(startersBlogFixture.createBlogPost))
.mockImplementationOnce(startersBlogFixture.initialSync)
.mockImplementationOnce(startersBlogFixture.createBlogPost)

const createdBlogEntry =
startersBlogFixture.createBlogPost.currentSyncData.entries[0]
const createdBlogEntry = startersBlogFixture.createBlogPost()
.currentSyncData.entries[0]
const createdBlogEntryIds = locales.map(locale =>
normalize.makeId({
spaceId: createdBlogEntry.sys.space.sys.id,
Expand Down Expand Up @@ -360,15 +361,15 @@ describe(`gatsby-node`, () => {
schema,
})
testIfContentTypesExists(
startersBlogFixture.createBlogPost.contentTypeItems
startersBlogFixture.createBlogPost().contentTypeItems
)
testIfEntriesExists(
startersBlogFixture.createBlogPost.currentSyncData.entries,
startersBlogFixture.createBlogPost.contentTypeItems,
startersBlogFixture.createBlogPost().currentSyncData.entries,
startersBlogFixture.createBlogPost().contentTypeItems,
locales
)
testIfAssetsExistsAndMatch(
startersBlogFixture.createBlogPost.currentSyncData.assets,
startersBlogFixture.createBlogPost().currentSyncData.assets,
locales
)

Expand All @@ -382,12 +383,12 @@ describe(`gatsby-node`, () => {
it(`should update a blogpost`, async () => {
const locales = [`en-US`, `nl`]
fetch
.mockReturnValueOnce(_.cloneDeep(startersBlogFixture.initialSync))
.mockReturnValueOnce(_.cloneDeep(startersBlogFixture.createBlogPost))
.mockReturnValueOnce(_.cloneDeep(startersBlogFixture.updateBlogPost))
.mockImplementationOnce(startersBlogFixture.initialSync)
.mockImplementationOnce(startersBlogFixture.createBlogPost)
.mockImplementationOnce(startersBlogFixture.updateBlogPost)

const updatedBlogEntry =
startersBlogFixture.updateBlogPost.currentSyncData.entries[0]
const updatedBlogEntry = startersBlogFixture.updateBlogPost()
.currentSyncData.entries[0]
const updatedBlogEntryIds = locales.map(locale =>
normalize.makeId({
spaceId: updatedBlogEntry.sys.space.sys.id,
Expand Down Expand Up @@ -442,15 +443,15 @@ describe(`gatsby-node`, () => {
})

testIfContentTypesExists(
startersBlogFixture.updateBlogPost.contentTypeItems
startersBlogFixture.updateBlogPost().contentTypeItems
)
testIfEntriesExists(
startersBlogFixture.updateBlogPost.currentSyncData.entries,
startersBlogFixture.updateBlogPost.contentTypeItems,
startersBlogFixture.updateBlogPost().currentSyncData.entries,
startersBlogFixture.updateBlogPost().contentTypeItems,
locales
)
testIfAssetsExistsAndMatch(
startersBlogFixture.updateBlogPost.currentSyncData.assets,
startersBlogFixture.updateBlogPost().currentSyncData.assets,
locales
)

Expand All @@ -465,12 +466,12 @@ describe(`gatsby-node`, () => {
it(`should remove a blogpost and update linkedNodes`, async () => {
const locales = [`en-US`, `nl`]
fetch
.mockReturnValueOnce(_.cloneDeep(startersBlogFixture.initialSync))
.mockReturnValueOnce(_.cloneDeep(startersBlogFixture.createBlogPost))
.mockReturnValueOnce(_.cloneDeep(startersBlogFixture.removeBlogPost))
.mockImplementationOnce(startersBlogFixture.initialSync)
.mockImplementationOnce(startersBlogFixture.createBlogPost)
.mockImplementationOnce(startersBlogFixture.removeBlogPost)

const removedBlogEntry =
startersBlogFixture.removeBlogPost.currentSyncData.deletedEntries[0]
const removedBlogEntry = startersBlogFixture.removeBlogPost()
.currentSyncData.deletedEntries[0]
const normalizedType = removedBlogEntry.sys.type.startsWith(`Deleted`)
? removedBlogEntry.sys.type.substring(`Deleted`.length)
: removedBlogEntry.sys.type
Expand Down Expand Up @@ -532,10 +533,10 @@ describe(`gatsby-node`, () => {
})

testIfContentTypesExists(
startersBlogFixture.removeBlogPost.contentTypeItems
startersBlogFixture.removeBlogPost().contentTypeItems
)
testIfEntriesDeleted(
startersBlogFixture.removeBlogPost.currentSyncData.assets,
startersBlogFixture.removeBlogPost().currentSyncData.assets,
locales
)

Expand All @@ -549,12 +550,12 @@ describe(`gatsby-node`, () => {
const locales = [`en-US`, `nl`]

fetch
.mockReturnValueOnce(startersBlogFixture.initialSync)
.mockReturnValueOnce(startersBlogFixture.createBlogPost)
.mockReturnValueOnce(startersBlogFixture.removeAsset)
.mockImplementationOnce(startersBlogFixture.initialSync)
.mockImplementationOnce(startersBlogFixture.createBlogPost)
.mockImplementationOnce(startersBlogFixture.removeAsset)

const removedAssetEntry =
startersBlogFixture.createBlogPost.currentSyncData.entries[0]
const removedAssetEntry = startersBlogFixture.createBlogPost()
.currentSyncData.entries[0]
const removedAssetEntryIds = locales.map(locale =>
normalize.makeId({
spaceId: removedAssetEntry.sys.space.sys.id,
Expand Down Expand Up @@ -598,7 +599,7 @@ describe(`gatsby-node`, () => {

// check if assets exists
testIfAssetsExists(
startersBlogFixture.removeAsset.currentSyncData.deletedAssets,
startersBlogFixture.removeAsset().currentSyncData.deletedAssets,
locales
)

Expand All @@ -615,14 +616,14 @@ describe(`gatsby-node`, () => {
schema,
})

testIfContentTypesExists(startersBlogFixture.removeAsset.contentTypeItems)
testIfContentTypesExists(startersBlogFixture.removeAsset().contentTypeItems)
testIfEntriesExists(
startersBlogFixture.removeAsset.currentSyncData.entries,
startersBlogFixture.removeAsset.contentTypeItems,
startersBlogFixture.removeAsset().currentSyncData.entries,
startersBlogFixture.removeAsset().contentTypeItems,
locales
)
testIfAssetsDeleted(
startersBlogFixture.removeAsset.currentSyncData.deletedAssets,
startersBlogFixture.removeAsset().currentSyncData.deletedAssets,
locales
)
})
Expand Down
20 changes: 13 additions & 7 deletions packages/gatsby-source-contentful/src/fetch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,17 @@ module.exports = async function contentfulFetch({
syncToken,
reporter,
pluginConfig,
parentSpan,
}) {
// Fetch articles.
console.time(`Fetch Contentful data`)
const fetchActivity = reporter.activityTimer(
`fetching data from Contentful`,
{
parentSpan,
}
)
fetchActivity.start()

console.log(`Starting to fetch data from Contentful`)
// Fetch articles.

const pageLimit = pluginConfig.get(`pageLimit`)
const contentfulClientOptions = {
Expand All @@ -33,7 +39,7 @@ module.exports = async function contentfulFetch({
let locales
let defaultLocale = `en-US`
try {
reporter.info(`Fetching default locale`)
reporter.verbose(`Fetching default locale`)
space = await client.getSpace()
let contentfulLocales = await client
.getLocales()
Expand All @@ -48,7 +54,7 @@ module.exports = async function contentfulFetch({
)}' were found but were filtered down to none.`
)
}
reporter.info(`Default locale is: ${defaultLocale}`)
reporter.verbose(`Default locale is: ${defaultLocale}`)
} catch (e) {
let details
let errors
Expand Down Expand Up @@ -110,7 +116,7 @@ ${formatPluginOptionsForCLI(pluginConfig.getOriginalPluginOptions(), errors)}`)
} catch (e) {
reporter.panic(`Error fetching content types`, e)
}
reporter.info(`Content types fetched ${contentTypes.items.length}`)
reporter.verbose(`Content types fetched ${contentTypes.items.length}`)

let contentTypeItems = contentTypes.items

Expand Down Expand Up @@ -138,7 +144,7 @@ ${formatPluginOptionsForCLI(pluginConfig.getOriginalPluginOptions(), errors)}`)
space,
}

console.timeEnd(`Fetch Contentful data`)
fetchActivity.end()

return result
}
Expand Down
34 changes: 22 additions & 12 deletions packages/gatsby-source-contentful/src/gatsby-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ const path = require(`path`)
const isOnline = require(`is-online`)
const _ = require(`lodash`)
const fs = require(`fs-extra`)
const { createClient } = require(`contentful`)

const normalize = require(`./normalize`)
const fetchData = require(`./fetch`)
const { createPluginConfig, validateOptions } = require(`./plugin-options`)
const { downloadContentfulAssets } = require(`./download-contentful-assets`)
const { createClient } = require(`contentful`)

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep it but I'm curious whether there's a particular reason for moving the import up. It implies it's relevant for side effects and I'm hoping it's not :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need it for resolving, so we need it always. Dependencies I need in all cases I do require on top. Would you require "on runtime" down there in line 200 when the client is created? 🤔

const conflictFieldPrefix = `contentful`

const CACHE_SYNC_KEY = `previousSyncData`
Expand Down Expand Up @@ -48,14 +50,11 @@ exports.sourceNodes = async (
getCache,
reporter,
schema,
parentSpan,
},
pluginOptions
) => {
const { createNode, deleteNode, touchNode, createTypes } = actions
const client = createClient({
space: `none`,
accessToken: `fake-access-token`,
})
const online = await isOnline()

// If the user knows they are offline, serve them cached result
Expand Down Expand Up @@ -117,6 +116,7 @@ exports.sourceNodes = async (
syncToken,
reporter,
pluginConfig,
parentSpan,
})

createTypes(`
Expand Down Expand Up @@ -156,7 +156,10 @@ exports.sourceNodes = async (
)
createTypes(gqlTypes)

console.time(`Process Contentful data`)
const processingActivity = reporter.activityTimer(`process Contentful data`, {
parentSpan,
})
processingActivity.start()

// Remove deleted entries and assets from cached sync data set
previousSyncData.entries = previousSyncData.entries.filter(
Expand Down Expand Up @@ -191,7 +194,10 @@ exports.sourceNodes = async (
const currentSyncDataRaw = _.cloneDeep(currentSyncData)

// Use the JS-SDK to resolve the entries and assets
const res = client.parseEntries({
const res = createClient({
space: `none`,
accessToken: `fake-access-token`,
}).parseEntries({
items: currentSyncData.entries,
includes: {
assets: currentSyncData.assets,
Expand Down Expand Up @@ -285,7 +291,7 @@ exports.sourceNodes = async (
cache.set(CACHE_SYNC_TOKEN, nextSyncToken),
])

reporter.info(`Building Contentful reference map`)
reporter.verbose(`Building Contentful reference map`)

// Create map of resolvable ids so we can check links against them while creating
// links.
Expand All @@ -309,7 +315,7 @@ exports.sourceNodes = async (
useNameForId: pluginConfig.get(`useNameForId`),
})

reporter.info(`Resolving Contentful references`)
reporter.verbose(`Resolving Contentful references`)

const newOrUpdatedEntries = []
entryList.forEach(entries => {
Expand Down Expand Up @@ -341,8 +347,12 @@ exports.sourceNodes = async (
}
})

console.timeEnd(`Process Contentful data`)
console.time(`Create Contentful nodes`)
processingActivity.end()

const creationActivity = reporter.activityTimer(`create Contentful nodes`, {
parentSpan,
})
creationActivity.start()

for (let i = 0; i < contentTypeItems.length; i++) {
const contentTypeItem = contentTypeItems[i]
Expand Down Expand Up @@ -393,7 +403,7 @@ exports.sourceNodes = async (
)
}

console.timeEnd(`Create Contentful nodes`)
creationActivity.end()

if (pluginConfig.get(`downloadLocal`)) {
reporter.info(`Download Contentful asset files`)
Expand Down
Loading