Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(zip-it-and-ship-it): create missing symlinks when archiveFormat=none #5836

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

ndhoule
Copy link
Contributor

@ndhoule ndhoule commented Sep 17, 2024

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: '../../../@[email protected]/node_modules/@netlify/sdk--extension-api-client',
  absoluteDestPath: '/home/ndhoule/dev/src/github.com/netlify/integration-csp/.netlify/functions-serve/trpc/node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@netlify/sdk--extension-api-client'
}
{
  targetPath: '../../../@[email protected]/node_modules/@netlify/sdk--extension-api-client',
  absoluteDestPath: '/home/ndhoule/dev/src/github.com/netlify/integration-csp/.netlify/functions-serve/trpc/node_modules/.pnpm/@[email protected]_@[email protected]_@[email protected]_@[email protected]._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.

@ndhoule ndhoule requested review from a team as code owners September 17, 2024 22:04
@ndhoule ndhoule changed the title input: fix(zip-it-and-ship-it): create missing symlinks when archiveFormat=none fix(zip-it-and-ship-it): create missing symlinks when archiveFormat=none Sep 17, 2024
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[email protected]/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[email protected]_@[email protected]/node_modules/@netlify/sdk--extension-api-client'
}
{
  targetPath: '../../../@netlify[email protected]/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[email protected]_@[email protected]_@[email protected]_@[email protected]._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.
@ndhoule ndhoule force-pushed the fix/create-all-symlinks-when-archive-format-none branch from b0ae4f7 to b47e049 Compare September 17, 2024 22:04
Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Could you add a test?

Copy link
Contributor

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

@ndhoule ndhoule force-pushed the fix/create-all-symlinks-when-archive-format-none branch 3 times, most recently from 70de5fb to f2c66b1 Compare September 18, 2024 17:10
@ndhoule ndhoule force-pushed the fix/create-all-symlinks-when-archive-format-none branch from f2c66b1 to ee659fb Compare September 18, 2024 18:03
@ndhoule
Copy link
Contributor Author

ndhoule commented Sep 18, 2024

Added a test.

Not sure why this macOS test is timing out, but it seems unrelated to my changes.

@ndhoule ndhoule merged commit 413b127 into main Sep 18, 2024
39 checks passed
@ndhoule ndhoule deleted the fix/create-all-symlinks-when-archive-format-none branch September 18, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants