[gatsby-remark-copy-linked-files] Fix bad copy location with route di…

…r and improve tests (#2875)

* Fix bad copy location with route dir and improve tests

* Switch to path.posix for joins in tests

packages/gatsby-remark-copy-linked-files/src/__tests__/index.js

@@ -2,6 +2,7 @@ jest.mock(`fs-extra`, () => {
   return {
     existsSync: () => false,
     copy: jest.fn(),
+    ensureDir: jest.fn(),
   }
 })
 const Remark = require(`remark`)
@@ -16,6 +17,8 @@ const remark = new Remark().data(`settings`, {
   pedantic: true,
 })
 
+const imageURL = markdownAST => markdownAST.children[0].children[0].url
+
 describe(`gatsby-remark-copy-linked-files`, () => {
   afterEach(() => {
     fsExtra.copy.mockReset()
@@ -129,32 +132,63 @@ describe(`gatsby-remark-copy-linked-files`, () => {
 
   describe(`options.destinationDir`, () => {
     const imagePath = `images/sample-image.gif`
-    const markdownAST = remark.parse(`![some absolute image](${imagePath})`)
 
     it(`throws an error if the destination directory is not within 'public'`, async () => {
+      const markdownAST = remark.parse(`![some absolute image](${imagePath})`)
       const invalidDestinationDir = `../destination`
-      expect.assertions(1)
+      expect.assertions(2)
       return plugin(
         { files: getFiles(imagePath), markdownAST, markdownNode, getNode },
         {
           destinationDir: invalidDestinationDir,
         }
       ).catch(e => {
         expect(e).toEqual(expect.stringContaining(invalidDestinationDir))
+        expect(fsExtra.copy).not.toHaveBeenCalled()
       })
     })
 
-    it(`doesn't throw an error if the destination directory is within 'public'`, async () => {
+    it(`copies file to destinationDir when supplied`, async () => {
+      const markdownAST = remark.parse(`![some absolute image](${imagePath})`)
       const validDestinationDir = `path/to/dir`
-      expect.assertions(1)
-      return plugin(
+      const expectedNewPath = path.posix.join(
+        process.cwd(),
+        `public`,
+        validDestinationDir,
+        `/undefined-undefined.gif`
+      )
+      expect.assertions(3)
+      await plugin(
         { files: getFiles(imagePath), markdownAST, markdownNode, getNode },
         {
           destinationDir: validDestinationDir,
         }
       ).then(v => {
-        const newPath = v[0]
-        expect(newPath).toContain(validDestinationDir)
+        expect(v).toBeDefined()
+        expect(fsExtra.copy).toHaveBeenCalledWith(imagePath, expectedNewPath)
+        expect(imageURL(markdownAST)).toEqual(
+          `/path/to/dir/undefined-undefined.gif`
+        )
+      })
+    })
+
+    it(`copies file to root dir when not supplied'`, async () => {
+      const markdownAST = remark.parse(`![some absolute image](${imagePath})`)
+      const expectedNewPath = path.posix.join(
+        process.cwd(),
+        `public`,
+        `/undefined-undefined.gif`
+      )
+      expect.assertions(3)
+      await plugin({
+        files: getFiles(imagePath),
+        markdownAST,
+        markdownNode,
+        getNode,
+      }).then(v => {
+        expect(v).toBeDefined()
+        expect(fsExtra.copy).toHaveBeenCalledWith(imagePath, expectedNewPath)
+        expect(imageURL(markdownAST)).toEqual(`/undefined-undefined.gif`)
       })
     })
   })

packages/gatsby-remark-copy-linked-files/src/index.js

@@ -5,32 +5,52 @@ const path = require(`path`)
 const _ = require(`lodash`)
 const cheerio = require(`cheerio`)
 const sizeOf = require(`image-size`)
-const pathIsInside = require(`path-is-inside`)
 
 const DEPLOY_DIR = `public`
 
-const invalidDestinationDirMessage = destinationDir =>
-  `[gatsby-remark-copy-linked-files] You have supplied an invalid destination directory. The destination directory must be within the '${
-    DEPLOY_DIR
-  }' directory, but was: '${destinationDir}'`
+const invalidDestinationDirMessage = dir =>
+  `[gatsby-remark-copy-linked-files You have supplied an invalid destination directory. The destination directory must be a child but was: ${
+    dir
+  }`
 
-const destinationDirIsValid = destinationDir =>
-  pathIsInside(path.join(DEPLOY_DIR, destinationDir), DEPLOY_DIR)
+// dir must be a child
+const destinationDirIsValid = dir => !path.relative(`./`, dir).startsWith(`..`)
+
+const validateDestinationDir = dir =>
+  !dir || (dir && destinationDirIsValid(dir))
+
+const newFileName = linkNode =>
+  `${linkNode.name}-${linkNode.internal.contentDigest}.${linkNode.extension}`
+
+const newPath = (linkNode, destinationDir) => {
+  if (destinationDir) {
+    return path.posix.join(
+      process.cwd(),
+      DEPLOY_DIR,
+      destinationDir,
+      newFileName(linkNode)
+    )
+  }
+  return path.posix.join(process.cwd(), DEPLOY_DIR, newFileName(linkNode))
+}
+
+const newLinkURL = (linkNode, destinationDir) => {
+  if (destinationDir) {
+    return path.posix.join(`/`, destinationDir, newFileName(linkNode))
+  }
+  return path.posix.join(`/`, newFileName(linkNode))
+}
 
 module.exports = (
   { files, markdownNode, markdownAST, getNode },
   pluginOptions = {}
 ) => {
   const defaults = {
     ignoreFileExtensions: [`png`, `jpg`, `jpeg`, `bmp`, `tiff`],
-    destinationDir: `/`,
   }
-
-  // Validate supplied destination directory
   const { destinationDir } = pluginOptions
-  if (destinationDir && !destinationDirIsValid(destinationDir)) {
+  if (!validateDestinationDir(destinationDir))
     return Promise.reject(invalidDestinationDirMessage(destinationDir))
-  }
 
   const options = _.defaults(pluginOptions, defaults)
 
@@ -53,24 +73,14 @@ module.exports = (
         return null
       })
       if (linkNode && linkNode.absolutePath) {
-        const newPath = path.posix.join(
-          process.cwd(),
-          DEPLOY_DIR,
-          options.destinationDir,
-          `${linkNode.internal.contentDigest}.${linkNode.extension}`
-        )
-        // Prevent uneeded copying
-        if (linkPath === newPath) {
-          return
-        }
+        const newFilePath = newPath(linkNode, options.destinationDir)
 
-        const relativePath = path.posix.join(
-          options.destinationDir,
-          `${linkNode.internal.contentDigest}.${linkNode.extension}`
-        )
-        link.url = `${relativePath}`
+        // Prevent uneeded copying
+        if (linkPath === newFilePath) return
 
-        filesToCopy.set(linkPath, newPath)
+        const linkURL = newLinkURL(linkNode, options.destinationDir)
+        link.url = linkURL
+        filesToCopy.set(linkPath, newFilePath)
       }
     }
   }
@@ -253,15 +263,16 @@ module.exports = (
   })
 
   return Promise.all(
-    Array.from(filesToCopy, async ([linkPath, newPath]) => {
-      if (!fsExtra.existsSync(newPath)) {
+    Array.from(filesToCopy, async ([linkPath, newFilePath]) => {
+      // Don't copy anything is the file already exists at the location.
+      if (!fsExtra.existsSync(newFilePath)) {
         try {
-          await fsExtra.copy(linkPath, newPath)
+          await fsExtra.ensureDir(path.dirname(newFilePath))
+          await fsExtra.copy(linkPath, newFilePath)
         } catch (err) {
-          console.error(`error copying file`, err)
+          console.error(`error copy ing file`, err)
         }
       }
-      return newPath
     })
   )
 }