fix(gatsby-recipes): fix NPMScript for parallel calls (#26349)

* Add tests for parallel NPMPackageJSON

* Add way to acquire/release locks on shared resources to safely read/write from them

* Also lock writes to gatsby-config.js

* Lock gatsby-config.js for site-metadata.js as well + make work when config doesn't have siteMetadata field

packages/gatsby-recipes/package.json

@@ -58,6 +58,7 @@
     "isomorphic-fetch": "^2.1.0",
     "jest-diff": "^25.5.0",
     "lodash": "^4.17.15",
+    "lock": "^1.0.0",
     "mitt": "^1.2.0",
     "mkdirp": "^0.5.1",
     "node-fetch": "^2.5.0",

packages/gatsby-recipes/src/providers/gatsby/__snapshots__/site-metadata.test.js.snap

@@ -337,3 +337,19 @@ module.exports = {
 ",
 }
 `;
+
+exports[`gatsby-plugin resource handles multiple parallel create calls 1`] = `
+Object {
+  "id": "husky",
+  "name": "husky",
+  "value": "\\"hi\\"",
+}
+`;
+
+exports[`gatsby-plugin resource handles multiple parallel create calls 2`] = `
+Object {
+  "id": "husky2",
+  "name": "husky2",
+  "value": "\\"hi\\"",
+}
+`;

packages/gatsby-recipes/src/providers/gatsby/plugin.js

@@ -9,6 +9,7 @@ const prettier = require(`prettier`)
 const resolveCwd = require(`resolve-cwd`)
 const { slash } = require(`gatsby-core-utils`)
 
+const lock = require(`../lock`)
 const getDiff = require(`../utils/get-diff`)
 const resourceSchema = require(`../resource-schema`)
 
@@ -194,6 +195,7 @@ class MissingInfoError extends Error {
 }
 
 const create = async ({ root }, { name, options, key }) => {
+  const release = await lock(`gatsby-config.js`)
   // TODO generalize this — it's for the demo.
   if (options?.accessToken === `(Known after install)`) {
     throw new MissingInfoError({ name, options, key })
@@ -206,7 +208,9 @@ const create = async ({ root }, { name, options, key }) => {
 
   await fs.writeFile(getConfigPath(root), code)
 
-  return await read({ root }, key || name)
+  const config = await read({ root }, key || name)
+  release()
+  return config
 }
 
 const read = async ({ root }, id) => {

packages/gatsby-recipes/src/providers/gatsby/plugin.test.js

@@ -166,8 +166,11 @@ describe(`gatsby-plugin resource`, () => {
       },
     }
 
-    await plugin.create(context, fooPlugin)
-    await plugin.create(context, barPlugin)
+    const createPromise1 = plugin.create(context, fooPlugin)
+    const createPromise2 = plugin.create(context, barPlugin)
+
+    await createPromise1
+    await createPromise2
 
     const barResult = await plugin.read(context, barPlugin.key)
     const fooResult = await plugin.read(context, fooPlugin.key)

packages/gatsby-recipes/src/providers/gatsby/site-metadata.js

@@ -1,10 +1,12 @@
 const fs = require(`fs-extra`)
 const path = require(`path`)
 const babel = require(`@babel/core`)
+const t = require(`@babel/types`)
 const declare = require(`@babel/helper-plugin-utils`).declare
 const Joi = require(`@hapi/joi`)
 const prettier = require(`prettier`)
 
+const lock = require(`../lock`)
 const getDiff = require(`../utils/get-diff`)
 const resourceSchema = require(`../resource-schema`)
 
@@ -78,6 +80,7 @@ module.exports = {
 }
 
 const create = async ({ root }, { name, value }) => {
+  const release = await lock(`gatsby-config.js`)
   const configSrc = await readConfigFile(root)
   const prettierConfig = await prettier.resolveConfig(root)
 
@@ -86,7 +89,9 @@ const create = async ({ root }, { name, value }) => {
 
   await fs.writeFile(getConfigPath(root), code)
 
-  return await read({ root }, name)
+  const resource = await read({ root }, name)
+  release()
+  return resource
 }
 
 const read = async ({ root }, id) => {
@@ -133,13 +138,16 @@ class BabelPluginSetSiteMetadataField {
               return
             }
 
+            let siteMetadataExists = false
             const siteMetadata = right.properties.find(
               p => p.key.name === `siteMetadata`
             )
 
-            if (!siteMetadata || !siteMetadata.value) return
-
-            const siteMetadataObj = getObjectFromNode(siteMetadata.value)
+            let siteMetadataObj = {}
+            if (siteMetadata?.value) {
+              siteMetadataExists = true
+              siteMetadataObj = getObjectFromNode(siteMetadata?.value)
+            }
 
             const valueType = typeof value
             const shouldParse =
@@ -155,14 +163,20 @@ class BabelPluginSetSiteMetadataField {
 
             const newSiteMetadata = newSiteMetadataTemplate.declarations[0].init
 
-            right.properties = right.properties.map(p => {
-              if (p.key.name !== `siteMetadata`) return p
-
-              return {
-                ...p,
-                value: newSiteMetadata,
-              }
-            })
+            if (siteMetadataExists) {
+              right.properties = right.properties.map(p => {
+                if (p.key.name !== `siteMetadata`) return p
+
+                return {
+                  ...p,
+                  value: newSiteMetadata,
+                }
+              })
+            } else {
+              right.properties.unshift(
+                t.objectProperty(t.identifier(`siteMetadata`), newSiteMetadata)
+              )
+            }
 
             path.stop()
           },

packages/gatsby-recipes/src/providers/gatsby/site-metadata.test.js

@@ -39,4 +39,35 @@ describe(`gatsby-plugin resource`, () => {
       partialUpdate: { name: `author`, value: `Velma` },
     })
   })
+
+  test(`handles multiple parallel create calls`, async () => {
+    const root = starterBlogRoot
+    const resultPromise = plugin.create(
+      {
+        root,
+      },
+      {
+        name: `husky`,
+        value: `hi`,
+      }
+    )
+    const result2Promise = plugin.create(
+      {
+        root,
+      },
+      {
+        name: `husky2`,
+        value: `hi`,
+      }
+    )
+
+    const result = await resultPromise
+    const result2 = await result2Promise
+
+    expect(result).toMatchSnapshot()
+    expect(result2).toMatchSnapshot()
+
+    await plugin.destroy({ root }, result)
+    await plugin.destroy({ root }, result2)
+  })
 })

packages/gatsby-recipes/src/providers/lock.js

@@ -0,0 +1,10 @@
+const lock = require(`lock`).Lock
+const lockInstance = lock()
+
+module.exports = resources =>
+  new Promise(resolve =>
+    lockInstance(resources, release => {
+      const releaseLock = release(() => {})
+      resolve(releaseLock)
+    })
+  )

packages/gatsby-recipes/src/providers/npm/__snapshots__/package-json.test.js.snap

@@ -78,6 +78,28 @@ Object {
 }
 `;
 
+exports[`packageJson resource handles multiple parallel create calls 1`] = `
+Object {
+  "_message": "Wrote key \\"husky\\" to package.json",
+  "id": "husky",
+  "name": "husky",
+  "value": "{
+  \\"hooks\\": {}
+}",
+}
+`;
+
+exports[`packageJson resource handles multiple parallel create calls 2`] = `
+Object {
+  "_message": "Wrote key \\"husky2\\" to package.json",
+  "id": "husky2",
+  "name": "husky2",
+  "value": "{
+  \\"hooks\\": {}
+}",
+}
+`;
+
 exports[`packageJson resource handles object values 1`] = `
 Object {
   "_message": "Wrote key \\"husky\\" to package.json",

packages/gatsby-recipes/src/providers/npm/__snapshots__/script.test.js.snap

@@ -55,3 +55,21 @@ Object {
   "newState": "\\"apple\\": \\"foot2\\"",
 }
 `;
+
+exports[`npm script resource handles multiple parallel create calls 1`] = `
+Object {
+  "_message": "Added script \\"husky\\" to your package.json",
+  "command": "hi",
+  "id": "husky",
+  "name": "husky",
+}
+`;
+
+exports[`npm script resource handles multiple parallel create calls 2`] = `
+Object {
+  "_message": "Added script \\"husky2\\" to your package.json",
+  "command": "hi",
+  "id": "husky2",
+  "name": "husky2",
+}
+`;

packages/gatsby-recipes/src/providers/npm/package-json.js

@@ -2,66 +2,10 @@ const fs = require(`fs-extra`)
 const path = require(`path`)
 const Joi = require(`@hapi/joi`)
 const getDiff = require(`../utils/get-diff`)
+const lock = require(`../lock`)
 
 const resourceSchema = require(`../resource-schema`)
 
-class Deferred {
-  constructor(name) {
-    this._promise = new Promise((resolve, reject) => {
-      // assign the resolve and reject functions to `this`
-      // making them usable on the class instance
-      this.resolve = resolve
-      this.reject = reject
-    })
-    this.name = name
-    // bind `then` and `catch` to implement the same interface as Promise
-    this.then = this._promise.then.bind(this._promise)
-    this.catch = this._promise.catch.bind(this._promise)
-    this[Symbol.toStringTag] = `Promise`
-  }
-}
-let writesQueue = new Map()
-let paused = false
-const checkWritesQueue = async root =>
-  new Promise((resolve, reject) => {
-    setTimeout(async () => {
-      if (writesQueue.size > 0) {
-        await processWritesQueue(root)
-        resolve()
-      } else {
-        paused = false
-      }
-    }, 100)
-  })
-
-const processWritesQueue = async root => {
-  const toProcess = [...writesQueue.entries()]
-  writesQueue = new Map()
-  const pkg = await readPackageJson(root)
-  toProcess.forEach(change => {
-    pkg[change[0]] = change[1].value
-  })
-
-  await writePackageJson(root, pkg)
-  toProcess.forEach(change => {
-    change[1].dfd.resolve()
-  })
-  await checkWritesQueue(root)
-}
-
-const enqueueWrite = (root, change) => {
-  const dfd = new Deferred(change[0])
-  writesQueue.set(change[0], { value: change[1], dfd })
-
-  // If we're not paused, write immediately
-  if (!paused) {
-    paused = true
-    processWritesQueue(root)
-  }
-
-  return dfd
-}
-
 const readPackageJson = async root => {
   const fullPath = path.join(root, `package.json`)
   const contents = await fs.readFile(fullPath, `utf8`)
@@ -76,9 +20,16 @@ const writePackageJson = async (root, obj) => {
 }
 
 const create = async ({ root }, { name, value }) => {
-  await enqueueWrite(root, [name, value])
+  const release = await lock(`package.json`)
+  const pkg = await readPackageJson(root)
+  pkg[name] = value
+
+  await writePackageJson(root, pkg)
+
+  const newPkg = await read({ root }, name)
 
-  return await read({ root }, name)
+  release()
+  return newPkg
 }
 
 const read = async ({ root }, id) => {

packages/gatsby-recipes/src/providers/npm/package-json.test.js

@@ -34,6 +34,36 @@ describe(`packageJson resource`, () => {
     })
   })
 
+  test(`handles multiple parallel create calls`, async () => {
+    const resultPromise = pkgJson.create(
+      {
+        root,
+      },
+      {
+        name: `husky`,
+        value: JSON.parse(initialValue),
+      }
+    )
+    const result2Promise = pkgJson.create(
+      {
+        root,
+      },
+      {
+        name: `husky2`,
+        value: JSON.parse(initialValue),
+      }
+    )
+
+    const result = await resultPromise
+    const result2 = await result2Promise
+
+    expect(result).toMatchSnapshot()
+    expect(result2).toMatchSnapshot()
+
+    await pkgJson.destroy({ root }, result)
+    await pkgJson.destroy({ root }, result2)
+  })
+
   test(`handles object values`, async () => {
     const result = await pkgJson.create(
       {

packages/gatsby-recipes/src/providers/npm/script.js

@@ -1,6 +1,7 @@
 const fs = require(`fs-extra`)
 const path = require(`path`)
 const Joi = require(`@hapi/joi`)
+const lock = require(`../lock`)
 
 const getDiff = require(`../utils/get-diff`)
 const resourceSchema = require(`../resource-schema`)
@@ -18,12 +19,15 @@ const writePackageJson = async (root, obj) => {
 }
 
 const create = async ({ root }, { name, command }) => {
+  const release = await lock(`package.json`)
   const pkg = await readPackageJson(root)
   pkg.scripts = pkg.scripts || {}
   pkg.scripts[name] = command
   await writePackageJson(root, pkg)
 
-  return await read({ root }, name)
+  const readPackagejson = await read({ root }, name)
+  release()
+  return readPackagejson
 }
 
 const read = async ({ root }, id) => {