From f01ba7e1218d81581320938d30ff9554019d5dc1 Mon Sep 17 00:00:00 2001 From: Pedr Browne Date: Fri, 10 Nov 2017 20:11:11 +0000 Subject: [PATCH] [gatsby-remark-copy-linked-files] Fix bad copy location with route dir and improve tests (#2875) * Fix bad copy location with route dir and improve tests * Switch to path.posix for joins in tests --- .../src/__tests__/index.js | 48 ++++++++++-- .../src/index.js | 77 +++++++++++-------- 2 files changed, 85 insertions(+), 40 deletions(-) diff --git a/packages/gatsby-remark-copy-linked-files/src/__tests__/index.js b/packages/gatsby-remark-copy-linked-files/src/__tests__/index.js index 82b422f091542..acc315be2f94f 100644 --- a/packages/gatsby-remark-copy-linked-files/src/__tests__/index.js +++ b/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,11 +132,11 @@ 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 }, { @@ -141,20 +144,51 @@ describe(`gatsby-remark-copy-linked-files`, () => { } ).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`) }) }) }) diff --git a/packages/gatsby-remark-copy-linked-files/src/index.js b/packages/gatsby-remark-copy-linked-files/src/index.js index d6df30e80adf0..27317048bcfee 100644 --- a/packages/gatsby-remark-copy-linked-files/src/index.js +++ b/packages/gatsby-remark-copy-linked-files/src/index.js @@ -5,17 +5,41 @@ 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 }, @@ -23,14 +47,10 @@ module.exports = ( ) => { 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 }) ) }