From 413b127a1b35951c7a256164e9ccfc706cc60bb4 Mon Sep 17 00:00:00 2001 From: Nathan Houle Date: Wed, 18 Sep 2024 16:34:13 -0700 Subject: [PATCH] fix(zip-it-and-ship-it): create missing symlinks when archiveFormat=none (#5836) * fix(zip-it-and-ship-it): create missing symlinks when archiveFormat=none This change updates zisi's node bundler to create missing symlinks in when `archiveFormat: 'none'` (the code path used by `netlify dev`). Currently, in projects where two or more symlinks resolve to the same target, we only ever create one symlink. In practice, how this usually presents a problem is when a package manager dedupes a dependency by symlinking it into two different packages; users end up getting a runtime error when their code hits the code path that tries to resolve the missing symlink. For example, given this scenario (which I took from a project that exhibits this issue): ``` { targetPath: '../../../@netlify+sdk--extension-api-client@2.2.0/node_modules/@netlify/sdk--extension-api-client', absoluteDestPath: '/home/ndhoule/dev/src/github.com/netlify/integration-csp/.netlify/functions-serve/trpc/node_modules/.pnpm/@netlify+sdk--ui-functions@1.0.4_@trpc+server@11.0.0-rc.477/node_modules/@netlify/sdk--extension-api-client' } { targetPath: '../../../@netlify+sdk--extension-api-client@2.2.0/node_modules/@netlify/sdk--extension-api-client', absoluteDestPath: '/home/ndhoule/dev/src/github.com/netlify/integration-csp/.netlify/functions-serve/trpc/node_modules/.pnpm/@netlify+sdk@2.3.1_@google-cloud+storage@5.20.5_@trpc+server@11.0.0-rc.477_@types+react@18.3._vp3jgimrs3u5kdeagmtefgn5zi/node_modules/@netlify/sdk--extension-api-client' } ``` ...only the second symlink is created. This is because as we walk through the list of files to output to the build directory, we only track a single symlink destination per target. Many symlinks can point at the same target, though, so we need to track multiple symlink destinations per target. I tested this out in my problem projects and it solved the problem. I took a stab at adding a test for this, but got a little lost in the test setup, which seems to have the `.zip` code path in mind rather than the `'none'` path. Happy to write some tests if someone can point me at a test setup. * test: add regression test for symlink bug --- .../src/runtimes/node/utils/zip.ts | 13 ++-- .../fixtures-esm/symlinked-deps/function.mjs | 6 ++ .../is-even-or-odd@1.0.0/node_modules/is-even | 1 + .../node_modules/is-even-or-odd/index.js | 3 + .../node_modules/is-even-or-odd/package.json | 8 +++ .../is-even-or-odd@1.0.0/node_modules/is-odd | 1 + .../node_modules/is-even/index.js | 2 + .../node_modules/is-even/package.json | 7 +++ .../.pnpm/is-even@1.0.0/node_modules/is-odd | 1 + .../is-odd@1.0.0/node_modules/is-odd/index.js | 1 + .../node_modules/is-odd/package.json | 4 ++ .../node_modules/is-even-or-odd | 1 + .../tests/symlinked_included_files.test.ts | 62 ++++++++++++++++++- 13 files changed, 104 insertions(+), 6 deletions(-) create mode 100644 packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/function.mjs create mode 120000 packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even create mode 100644 packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even-or-odd/index.js create mode 100644 packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even-or-odd/package.json create mode 120000 packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-odd create mode 100644 packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even@1.0.0/node_modules/is-even/index.js create mode 100644 packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even@1.0.0/node_modules/is-even/package.json create mode 120000 packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even@1.0.0/node_modules/is-odd create mode 100644 packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-odd@1.0.0/node_modules/is-odd/index.js create mode 100644 packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-odd@1.0.0/node_modules/is-odd/package.json create mode 120000 packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/is-even-or-odd diff --git a/packages/zip-it-and-ship-it/src/runtimes/node/utils/zip.ts b/packages/zip-it-and-ship-it/src/runtimes/node/utils/zip.ts index 01ed72d52e..657e0583e5 100644 --- a/packages/zip-it-and-ship-it/src/runtimes/node/utils/zip.ts +++ b/packages/zip-it-and-ship-it/src/runtimes/node/utils/zip.ts @@ -129,7 +129,7 @@ const createDirectory = async function ({ addBootstrapFile(srcFiles, aliases) } - const symlinks = new Map() + const symlinks = new Map>() // Copying source files. await pMap( @@ -156,7 +156,8 @@ const createDirectory = async function ({ if (stat.isSymbolicLink()) { const targetPath = await readLink(srcFile) - symlinks.set(targetPath, absoluteDestPath) + // Two symlinks may point at the same target path, so keep a list of symlinks to create. + symlinks.set(targetPath, (symlinks.get(targetPath) ?? new Set()).add(absoluteDestPath)) return } @@ -168,9 +169,11 @@ const createDirectory = async function ({ await pMap( [...symlinks.entries()], - async ([target, path]) => { - await mkdir(dirname(path), { recursive: true }) - await symlink(target, path) + async ([target, paths]) => { + for (const path of paths) { + await mkdir(dirname(path), { recursive: true }) + await symlink(target, path) + } }, { concurrency: COPY_FILE_CONCURRENCY, diff --git a/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/function.mjs b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/function.mjs new file mode 100644 index 0000000000..cbef870f47 --- /dev/null +++ b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/function.mjs @@ -0,0 +1,6 @@ +export default async () => + new Response('

Hello world

', { + headers: { + 'content-type': 'text/html', + }, + }) diff --git a/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even new file mode 120000 index 0000000000..a1c06cb4e2 --- /dev/null +++ b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even @@ -0,0 +1 @@ +../../is-even@1.0.0/node_modules/is-even \ No newline at end of file diff --git a/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even-or-odd/index.js b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even-or-odd/index.js new file mode 100644 index 0000000000..46b48ee97a --- /dev/null +++ b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even-or-odd/index.js @@ -0,0 +1,3 @@ +const isOdd = require('is-odd') +const isEven = require('is-even') +module.exports = (number) => isEven(number) || isOdd(number) diff --git a/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even-or-odd/package.json b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even-or-odd/package.json new file mode 100644 index 0000000000..96d3dc910e --- /dev/null +++ b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even-or-odd/package.json @@ -0,0 +1,8 @@ +{ + "name": "is-even-or-odd", + "main": "index.js", + "dependencies": { + "is-even": "^1.0.0", + "is-odd": "^1.0.0" + } +} diff --git a/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-odd b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-odd new file mode 120000 index 0000000000..be987a530a --- /dev/null +++ b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-odd @@ -0,0 +1 @@ +../../is-odd@1.0.0/node_modules/is-odd \ No newline at end of file diff --git a/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even@1.0.0/node_modules/is-even/index.js b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even@1.0.0/node_modules/is-even/index.js new file mode 100644 index 0000000000..d53967411c --- /dev/null +++ b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even@1.0.0/node_modules/is-even/index.js @@ -0,0 +1,2 @@ +const isOdd = require('is-odd') +module.exports = (number) => !isOdd(number) diff --git a/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even@1.0.0/node_modules/is-even/package.json b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even@1.0.0/node_modules/is-even/package.json new file mode 100644 index 0000000000..0476882fbd --- /dev/null +++ b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even@1.0.0/node_modules/is-even/package.json @@ -0,0 +1,7 @@ +{ + "name": "is-even", + "main": "index.js", + "dependencies": { + "is-odd": "^1.0.0" + } +} diff --git a/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even@1.0.0/node_modules/is-odd b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even@1.0.0/node_modules/is-odd new file mode 120000 index 0000000000..be987a530a --- /dev/null +++ b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-even@1.0.0/node_modules/is-odd @@ -0,0 +1 @@ +../../is-odd@1.0.0/node_modules/is-odd \ No newline at end of file diff --git a/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-odd@1.0.0/node_modules/is-odd/index.js b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-odd@1.0.0/node_modules/is-odd/index.js new file mode 100644 index 0000000000..d93533856d --- /dev/null +++ b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-odd@1.0.0/node_modules/is-odd/index.js @@ -0,0 +1 @@ +module.exports = (number) => number % 2 === 1 diff --git a/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-odd@1.0.0/node_modules/is-odd/package.json b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-odd@1.0.0/node_modules/is-odd/package.json new file mode 100644 index 0000000000..a1098c13ac --- /dev/null +++ b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/.pnpm/is-odd@1.0.0/node_modules/is-odd/package.json @@ -0,0 +1,4 @@ +{ + "name": "is-odd", + "main": "index.js" +} \ No newline at end of file diff --git a/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/is-even-or-odd b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/is-even-or-odd new file mode 120000 index 0000000000..a03f0ab4d1 --- /dev/null +++ b/packages/zip-it-and-ship-it/tests/fixtures-esm/symlinked-deps/node_modules/is-even-or-odd @@ -0,0 +1 @@ +.pnpm/is-even-or-odd@1.0.0/node_modules/is-even-or-odd \ No newline at end of file diff --git a/packages/zip-it-and-ship-it/tests/symlinked_included_files.test.ts b/packages/zip-it-and-ship-it/tests/symlinked_included_files.test.ts index 385e611d38..c6a8eeec4f 100644 --- a/packages/zip-it-and-ship-it/tests/symlinked_included_files.test.ts +++ b/packages/zip-it-and-ship-it/tests/symlinked_included_files.test.ts @@ -1,6 +1,6 @@ import { readdir } from 'fs/promises' import { platform } from 'os' -import { join } from 'path' +import { join, sep } from 'path' import decompress from 'decompress' import { dir as getTmpDir } from 'tmp-promise' @@ -69,6 +69,66 @@ test.skipIf(platform() === 'win32')('Symlinked directories from `includedFiles` }) }) +// Regression test for https://github.com/netlify/build/pull/5836 +test('preserves multiple symlinks that link to the same target', async () => { + const { path: tmpDir } = await getTmpDir({ prefix: 'zip-it-test' }) + const basePath = join(FIXTURES_ESM_DIR, 'symlinked-deps') + const mainFile = join(basePath, 'function.mjs') + + // Two symlinks that point at `node_modules/.pnpm/is-odd@1.0.0/node_modules/is-odd`: + // + // - `node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-odd` + // - `node_modules/.pnpm/is-even@1.0.0/node_modules/is-odd` + expect(await readDirWithType(basePath)).toEqual({ + 'function.mjs': false, + ['node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even'.replace(/\//g, sep)]: true, + ['node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even-or-odd/index.js'.replace(/\//g, sep)]: false, + ['node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even-or-odd/package.json'.replace(/\//g, sep)]: false, + ['node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-odd'.replace(/\//g, sep)]: true, + ['node_modules/.pnpm/is-even@1.0.0/node_modules/is-even/index.js'.replace(/\//g, sep)]: false, + ['node_modules/.pnpm/is-even@1.0.0/node_modules/is-even/package.json'.replace(/\//g, sep)]: false, + ['node_modules/.pnpm/is-even@1.0.0/node_modules/is-odd'.replace(/\//g, sep)]: true, + ['node_modules/.pnpm/is-odd@1.0.0/node_modules/is-odd/index.js'.replace(/\//g, sep)]: false, + ['node_modules/.pnpm/is-odd@1.0.0/node_modules/is-odd/package.json'.replace(/\//g, sep)]: false, + ['node_modules/is-even-or-odd'.replace(/\//g, sep)]: true, + }) + + await zipFunction(mainFile, tmpDir, { + archiveFormat: ARCHIVE_FORMAT.NONE, + basePath, + config: { + '*': { + includedFiles: ['**'], + }, + }, + featureFlags: { + zisi_fix_symlinks: true, + }, + repositoryRoot: basePath, + systemLog: console.log, + debug: true, + internalSrcFolder: undefined, + }) + + // Test to be sure we've made both symlinks, not just one of them + expect(await readDirWithType(join(tmpDir, 'function'))).toEqual({ + '___netlify-bootstrap.mjs': false, + '___netlify-entry-point.mjs': false, + '___netlify-telemetry.mjs': false, + 'function.mjs': false, + ['node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even'.replace(/\//g, sep)]: true, + ['node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even-or-odd/index.js'.replace(/\//g, sep)]: false, + ['node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-even-or-odd/package.json'.replace(/\//g, sep)]: false, + ['node_modules/.pnpm/is-even-or-odd@1.0.0/node_modules/is-odd'.replace(/\//g, sep)]: true, + ['node_modules/.pnpm/is-even@1.0.0/node_modules/is-even/index.js'.replace(/\//g, sep)]: false, + ['node_modules/.pnpm/is-even@1.0.0/node_modules/is-even/package.json'.replace(/\//g, sep)]: false, + ['node_modules/.pnpm/is-even@1.0.0/node_modules/is-odd'.replace(/\//g, sep)]: true, + ['node_modules/.pnpm/is-odd@1.0.0/node_modules/is-odd/index.js'.replace(/\//g, sep)]: false, + ['node_modules/.pnpm/is-odd@1.0.0/node_modules/is-odd/package.json'.replace(/\//g, sep)]: false, + ['node_modules/is-even-or-odd'.replace(/\//g, sep)]: true, + }) +}) + test('symlinks in subdir of `includedFiles` are copied over successfully', async () => { const { path: tmpDir } = await getTmpDir({ prefix: 'zip-it-test' }) const basePath = join(FIXTURES_ESM_DIR, 'symlinked-bin')