From 071d50d665c6f5542e29286eaef4dc54a61bb4d8 Mon Sep 17 00:00:00 2001 From: Peter van der Zee <209817+pvdz@users.noreply.github.com> Date: Tue, 10 Nov 2020 12:01:51 +0100 Subject: [PATCH] fix(gatsby-transformer-remark): wait for cache promises before returning (#27871) * chore(gatsby-transformer-remark): refactor async logic to allow serial mode * Refactor the rest while we are here * Move addSlugToUrl out of the inner-inner function * arrow -> nfe * fix: fake/placeholder cache get/set should be async too * Be explicit and greppable about GatsbyCache * Refactor more cases to proper async/await * Make test not assume a non-async response * await promise like all the others --- .../src/__tests__/extend-node.js | 32 +- .../src/extend-node-type.js | 310 +++++++++--------- .../src/utils/__tests__/api-runner-node.js | 4 +- packages/gatsby/src/utils/api-runner-node.js | 5 +- packages/gatsby/src/utils/cache.ts | 8 +- packages/gatsby/src/utils/get-cache.ts | 8 +- 6 files changed, 194 insertions(+), 173 deletions(-) diff --git a/packages/gatsby-transformer-remark/src/__tests__/extend-node.js b/packages/gatsby-transformer-remark/src/__tests__/extend-node.js index 0bdce94709ff1..a2a85eadee18c 100644 --- a/packages/gatsby-transformer-remark/src/__tests__/extend-node.js +++ b/packages/gatsby-transformer-remark/src/__tests__/extend-node.js @@ -30,8 +30,9 @@ async function queryResult( { type: { name: `MarkdownRemark` }, cache: { - get: () => null, - set: () => null, + // GatsbyCache + get: async () => null, + set: async () => null, }, getNodesByType: type => [], ...additionalParameters, @@ -104,23 +105,24 @@ const bootstrapTest = ( it(label, async done => { node.content = content - const createNode = markdownNode => { - queryResult([markdownNode], query, { + async function createNode(markdownNode) { + const result = await queryResult([markdownNode], query, { additionalParameters, pluginOptions, - }).then(result => { - if (result.errors) { - done.fail(result.errors) - } - - try { - test(result.data.listNode[0]) - done() - } catch (err) { - done.fail(err) - } }) + + if (result.errors) { + done.fail(result.errors) + } + + try { + test(result.data.listNode[0]) + done() + } catch (err) { + done.fail(err) + } } + const createParentChildLink = jest.fn() const actions = { createNode, createParentChildLink } const createNodeId = jest.fn() diff --git a/packages/gatsby-transformer-remark/src/extend-node-type.js b/packages/gatsby-transformer-remark/src/extend-node-type.js index ac7744d3c95ac..b8e2e3cd1f807 100644 --- a/packages/gatsby-transformer-remark/src/extend-node-type.js +++ b/packages/gatsby-transformer-remark/src/extend-node-type.js @@ -80,7 +80,7 @@ const headingLevels = [...Array(6).keys()].reduce((acc, i) => { return acc }, {}) -module.exports = ( +module.exports = function remarkExtendNodeType( { type, basePath, @@ -92,7 +92,7 @@ module.exports = ( ...rest }, pluginOptions -) => { +) { if (type.name !== `MarkdownRemark`) { return {} } @@ -144,30 +144,35 @@ module.exports = ( async function getAST(markdownNode) { const cacheKey = astCacheKey(markdownNode) + + const promise = ASTPromiseMap.get(cacheKey) + if (promise) { + // We are already generating AST, so let's wait for it + return promise + } + const cachedAST = await cache.get(cacheKey) if (cachedAST) { return cachedAST - } else if (ASTPromiseMap.has(cacheKey)) { - // We are already generating AST, so let's wait for it - return await ASTPromiseMap.get(cacheKey) - } else { - const ASTGenerationPromise = getMarkdownAST(markdownNode) - ASTGenerationPromise.then(markdownAST => { - ASTPromiseMap.delete(cacheKey) - return cache.set(cacheKey, markdownAST) - }).catch(err => { - ASTPromiseMap.delete(cacheKey) - return err - }) - // Save new AST to cache and return - // We can now release promise, as we cached result - ASTPromiseMap.set(cacheKey, ASTGenerationPromise) - return ASTGenerationPromise + } + + const ASTGenerationPromise = getMarkdownAST(markdownNode) + ASTPromiseMap.set(cacheKey, ASTGenerationPromise) + + // Return the promise that will cache the result if good, and deletes the promise from local cache either way + // Note that if this cache hits for another parse, it won't have to wait for the cache + try { + const markdownAST = await ASTGenerationPromise + + await cache.set(cacheKey, markdownAST) + + return markdownAST + } finally { + ASTPromiseMap.delete(cacheKey) } } - // Parse a markdown string and its AST representation, - // applying the remark plugins if necesserary + // Parse a markdown string and its AST representation, applying the remark plugins if necessary async function parseString(string, markdownNode) { // compiler to inject in the remark plugins // so that they can use our parser/generator @@ -241,10 +246,10 @@ module.exports = ( // before parsing its content // // Use Bluebird's Promise function "each" to run remark plugins serially. - await Promise.each(pluginOptions.plugins, plugin => { + for (const plugin of pluginOptions.plugins) { const requiredPlugin = require(plugin.resolve) - if (_.isFunction(requiredPlugin.mutateSource)) { - return requiredPlugin.mutateSource( + if (typeof requiredPlugin.mutateSource === `function`) { + await requiredPlugin.mutateSource( { markdownNode, files: fileNodes, @@ -256,88 +261,98 @@ module.exports = ( }, plugin.pluginOptions ) - } else { - return Promise.resolve() } - }) + } return parseString(markdownNode.internal.content, markdownNode) } async function getHeadings(markdownNode) { - const cachedHeadings = await cache.get(headingsCacheKey(markdownNode)) + const markdownCacheKey = headingsCacheKey(markdownNode) + const cachedHeadings = await cache.get(markdownCacheKey) if (cachedHeadings) { return cachedHeadings - } else { - const ast = await getAST(markdownNode) - const headings = select(ast, `heading`).map(heading => { - return { - id: getHeadingID(heading), - value: mdastToString(heading), - depth: heading.depth, - } - }) + } - cache.set(headingsCacheKey(markdownNode), headings) - return headings + const ast = await getAST(markdownNode) + const headings = select(ast, `heading`).map(heading => { + return { + id: getHeadingID(heading), + value: mdastToString(heading), + depth: heading.depth, + } + }) + + await cache.set(markdownCacheKey, headings) + return headings + } + + function addSlugToUrl(markdownNode, slugField, appliedTocOptions, node) { + if (node.url) { + if (slugField === undefined) { + console.warn( + `Skipping TableOfContents. Field '${appliedTocOptions.pathToSlugField}' missing from markdown node` + ) + return null + } + + node.url = [basePath, slugField, node.url] + .join(`/`) + .replace(/\/\//g, `/`) + } + + if (node.children) { + node.children = node.children + .map(node => + addSlugToUrl(markdownNode, slugField, appliedTocOptions, node) + ) + .filter(Boolean) } + + return node } async function getTableOfContents(markdownNode, gqlTocOptions) { // fetch defaults let appliedTocOptions = { ...tocOptions, ...gqlTocOptions } + + const tocKey = tableOfContentsCacheKey(markdownNode, appliedTocOptions) + // get cached toc - const cachedToc = await cache.get( - tableOfContentsCacheKey(markdownNode, appliedTocOptions) - ) + const cachedToc = await cache.get(tocKey) if (cachedToc) { return cachedToc - } else { - const ast = await getAST(markdownNode) - const tocAst = mdastToToc(ast, appliedTocOptions) + } - let toc - if (tocAst.map) { - const addSlugToUrl = function (node) { - if (node.url) { - if ( - _.get(markdownNode, appliedTocOptions.pathToSlugField) === - undefined - ) { - console.warn( - `Skipping TableOfContents. Field '${appliedTocOptions.pathToSlugField}' missing from markdown node` - ) - return null - } - node.url = [ - basePath, - _.get(markdownNode, appliedTocOptions.pathToSlugField), - node.url, - ] - .join(`/`) - .replace(/\/\//g, `/`) - } - if (node.children) { - node.children = node.children - .map(node => addSlugToUrl(node)) - .filter(Boolean) - } + const ast = await getAST(markdownNode, cache) + const tocAst = mdastToToc(ast, appliedTocOptions) - return node - } - if (appliedTocOptions.absolute) { - tocAst.map = addSlugToUrl(tocAst.map) - } + let toc = `` + if (tocAst.map) { + if (appliedTocOptions.absolute) { + const slugField = _.get( + markdownNode, + appliedTocOptions.pathToSlugField + ) + + tocAst.map = addSlugToUrl( + markdownNode, + slugField, + appliedTocOptions, + tocAst.map + ) + } + // addSlugToUrl may clear the map + if (tocAst.map) { toc = hastToHTML(toHAST(tocAst.map, { allowDangerousHTML: true }), { allowDangerousHTML: true, }) - } else { - toc = `` } - cache.set(tableOfContentsCacheKey(markdownNode, appliedTocOptions), toc) - return toc } + + await cache.set(tocKey, toc) + return toc } function markdownASTToHTMLAst(ast) { @@ -352,11 +367,11 @@ module.exports = ( if (cachedAst) { return cachedAst } else { - const ast = await getAST(markdownNode) + const ast = await getAST(markdownNode, cache) const htmlAst = markdownASTToHTMLAst(ast) // Save new HTML AST to cache and return - cache.set(htmlAstCacheKey(markdownNode), htmlAst) + await cache.set(htmlAstCacheKey(markdownNode), htmlAst) return htmlAst } } @@ -373,7 +388,7 @@ module.exports = ( }) // Save new HTML to cache - cache.set(htmlCacheKey(markdownNode), html) + await cache.set(htmlCacheKey(markdownNode), html) return html } @@ -391,6 +406,7 @@ module.exports = ( nextNode.type === `raw` && nextNode.value === excerptSeparator ) } + if (!fullAST.children.length) { return fullAST } @@ -400,6 +416,7 @@ module.exports = ( return totalExcerptSoFar && totalExcerptSoFar.length > pruneLength }) const unprunedExcerpt = getConcatenatedValue(excerptAST) + if ( !unprunedExcerpt || (pruneLength && unprunedExcerpt.length < pruneLength) @@ -423,6 +440,7 @@ module.exports = ( omission: `…`, }) } + return excerptAST } @@ -438,10 +456,10 @@ module.exports = ( truncate, excerptSeparator, }) - const html = hastToHTML(excerptAST, { + + return hastToHTML(excerptAST, { allowDangerousHTML: true, }) - return html } async function getExcerptMarkdown( @@ -454,14 +472,15 @@ module.exports = ( if (excerptSeparator && markdownNode.excerpt !== ``) { return markdownNode.excerpt } + const ast = await getMarkdownAST(markdownNode) const excerptAST = await getExcerptAst(ast, markdownNode, { pruneLength, truncate, excerptSeparator, }) - var excerptMarkdown = unified().use(stringify).stringify(excerptAST) - return excerptMarkdown + + return unified().use(stringify).stringify(excerptAST) } async function getExcerptPlain( @@ -470,40 +489,41 @@ module.exports = ( truncate, excerptSeparator ) { - const text = await getAST(markdownNode).then(ast => { - const excerptNodes = [] - let isBeforeSeparator = true - visit( - ast, - node => isBeforeSeparator, - node => { - if (excerptSeparator && node.value === excerptSeparator) { - isBeforeSeparator = false - } else if (node.type === `text` || node.type === `inlineCode`) { - excerptNodes.push(node.value) - } else if (node.type === `image`) { - excerptNodes.push(node.alt) - } else if (SpaceMarkdownNodeTypesSet.has(node.type)) { - // Add a space when encountering one of these node types. - excerptNodes.push(` `) - } + const ast = await getAST(markdownNode) + + const excerptNodes = [] + let isBeforeSeparator = true + visit( + ast, + node => isBeforeSeparator, + node => { + if (excerptSeparator && node.value === excerptSeparator) { + isBeforeSeparator = false + } else if (node.type === `text` || node.type === `inlineCode`) { + excerptNodes.push(node.value) + } else if (node.type === `image`) { + excerptNodes.push(node.alt) + } else if (SpaceMarkdownNodeTypesSet.has(node.type)) { + // Add a space when encountering one of these node types. + excerptNodes.push(` `) } - ) + } + ) - const excerptText = excerptNodes.join(``).trim() + const excerptText = excerptNodes.join(``).trim() - if (excerptSeparator && !isBeforeSeparator) { - return excerptText - } - if (!truncate) { - return prune(excerptText, pruneLength, `…`) - } - return _.truncate(excerptText, { - length: pruneLength, - omission: `…`, - }) + if (excerptSeparator && !isBeforeSeparator) { + return excerptText + } + + if (!truncate) { + return prune(excerptText, pruneLength, `…`) + } + + return _.truncate(excerptText, { + length: pruneLength, + omission: `…`, }) - return text } async function getExcerpt( @@ -517,7 +537,9 @@ module.exports = ( truncate, excerptSeparator ) - } else if (format === `MARKDOWN`) { + } + + if (format === `MARKDOWN`) { return getExcerptMarkdown( markdownNode, pruneLength, @@ -525,6 +547,7 @@ module.exports = ( excerptSeparator ) } + return getExcerptPlain( markdownNode, pruneLength, @@ -536,17 +559,16 @@ module.exports = ( return resolve({ html: { type: `String`, - resolve(markdownNode) { + async resolve(markdownNode) { return getHTML(markdownNode) }, }, htmlAst: { type: `JSON`, - resolve(markdownNode) { - return getHTMLAst(markdownNode).then(ast => { - const strippedAst = stripPosition(_.clone(ast), true) - return hastReparseRaw(strippedAst) - }) + async resolve(markdownNode) { + const ast = await getHTMLAst(markdownNode) + const strippedAst = stripPosition(_.clone(ast), true) + return hastReparseRaw(strippedAst) }, }, excerpt: { @@ -586,19 +608,15 @@ module.exports = ( defaultValue: false, }, }, - resolve(markdownNode, { pruneLength, truncate }) { - return getHTMLAst(markdownNode) - .then(fullAST => - getExcerptAst(fullAST, markdownNode, { - pruneLength, - truncate, - excerptSeparator: pluginOptions.excerpt_separator, - }) - ) - .then(ast => { - const strippedAst = stripPosition(_.clone(ast), true) - return hastReparseRaw(strippedAst) - }) + async resolve(markdownNode, { pruneLength, truncate }) { + const fullAST = await getHTMLAst(markdownNode) + const ast = await getExcerptAst(fullAST, markdownNode, { + pruneLength, + truncate, + excerptSeparator: pluginOptions.excerpt_separator, + }) + const strippedAst = stripPosition(_.clone(ast), true) + return hastReparseRaw(strippedAst) }, }, headings: { @@ -606,20 +624,20 @@ module.exports = ( args: { depth: `MarkdownHeadingLevels`, }, - resolve(markdownNode, { depth }) { - return getHeadings(markdownNode).then(headings => { - const level = depth && headingLevels[depth] - if (typeof level === `number`) { - headings = headings.filter(heading => heading.depth === level) - } - return headings - }) + async resolve(markdownNode, { depth }) { + let headings = await getHeadings(markdownNode) + const level = depth && headingLevels[depth] + if (typeof level === `number`) { + headings = headings.filter(heading => heading.depth === level) + } + return headings }, }, timeToRead: { type: `Int`, - resolve(markdownNode) { - return getHTML(markdownNode).then(timeToRead) + async resolve(markdownNode) { + const r = await getHTML(markdownNode) + return timeToRead(r) }, }, tableOfContents: { diff --git a/packages/gatsby/src/utils/__tests__/api-runner-node.js b/packages/gatsby/src/utils/__tests__/api-runner-node.js index 85ee323b52489..a44d695ebdf0b 100644 --- a/packages/gatsby/src/utils/__tests__/api-runner-node.js +++ b/packages/gatsby/src/utils/__tests__/api-runner-node.js @@ -190,8 +190,8 @@ it(`Doesn't initialize cache in onPreInit API`, async () => { `test-plugin-on-preinit-fails/gatsby-node`, () => { return { - onPreInit: ({ cache }) => { - cache.get(`foo`) + onPreInit: async ({ cache }) => { + await cache.get(`foo`) }, } }, diff --git a/packages/gatsby/src/utils/api-runner-node.js b/packages/gatsby/src/utils/api-runner-node.js index e8c43fa5bb0c4..7e96729fef0a6 100644 --- a/packages/gatsby/src/utils/api-runner-node.js +++ b/packages/gatsby/src/utils/api-runner-node.js @@ -275,10 +275,11 @@ const getUninitializedCache = plugin => { (plugin && plugin !== `default-site-plugin` ? ` (called in ${plugin})` : ``) return { - get() { + // GatsbyCache + async get() { throw new Error(message) }, - set() { + async set() { throw new Error(message) }, } diff --git a/packages/gatsby/src/utils/cache.ts b/packages/gatsby/src/utils/cache.ts index 97046f64f5f49..14241aad53478 100644 --- a/packages/gatsby/src/utils/cache.ts +++ b/packages/gatsby/src/utils/cache.ts @@ -16,7 +16,7 @@ interface ICacheProperties { store?: Store } -export default class Cache { +export default class GatsbyCache { public name: string public store: Store public directory: string @@ -28,7 +28,7 @@ export default class Cache { this.directory = path.join(process.cwd(), `.cache/caches/${name}`) } - init(): Cache { + init(): GatsbyCache { fs.ensureDirSync(this.directory) const configs: Array = [ @@ -58,7 +58,7 @@ export default class Cache { return new Promise(resolve => { if (!this.cache) { throw new Error( - `Cache wasn't initialised yet, please run the init method first` + `GatsbyCache wasn't initialised yet, please run the init method first` ) } this.cache.get(key, (err, res) => { @@ -75,7 +75,7 @@ export default class Cache { return new Promise(resolve => { if (!this.cache) { throw new Error( - `Cache wasn't initialised yet, please run the init method first` + `GatsbyCache wasn't initialised yet, please run the init method first` ) } this.cache.set(key, value, args, err => { diff --git a/packages/gatsby/src/utils/get-cache.ts b/packages/gatsby/src/utils/get-cache.ts index b37fe9e33059f..f8e3faa2b7f63 100644 --- a/packages/gatsby/src/utils/get-cache.ts +++ b/packages/gatsby/src/utils/get-cache.ts @@ -1,11 +1,11 @@ -import Cache from "./cache" +import GatsbyCache from "./cache" -const caches = new Map() +const caches = new Map() -export const getCache = (name: string): Cache => { +export const getCache = (name: string): GatsbyCache => { let cache = caches.get(name) if (!cache) { - cache = new Cache({ name }).init() + cache = new GatsbyCache({ name }).init() caches.set(name, cache) } return cache