fix: follow symlinks w/ `netlify deploy` #5650

ajschmidt8 · 2023-04-23T22:39:39Z

Summary

There's a discrepancy between deploys that happen via Netlify's auto-deploys and the netlify deploy CLI command. Auto-deploys follow symlinks while the netlify deploy CLI command does not.

This PR resolves that discrepancy by ensuring the netlify deploy command does follow symlinks.

As mentioned by @tinfoil-knight in this comment, the fast-glob library follows symlinks by default.

Therefore, I've replaced the folder-walker dependency that was previously used with fast-glob.

The APIs and return values of these libraries are different, so I've adjusted the code accordingly.

Further, I've replaced the streams implementations in src/utils/deploy/hash-files.mjs with async/await promises (and also updated src/utils/deploy/hash-fns.mjs for consistency).

This seemed like the preferred approach based on some of the TODO comments in src/utils/deploy/hasher-segments.mjs (which has now been deleted).

This switch also enabled the removal of the following dependencies:

pump
from2-array
flush-write-stream
parallel-transform

Finally, tests were added/updated according to these changes.

For us to review and ship your PR efficiently, please perform the following steps:

Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
Read the contribution guidelines 📖. This ensures your code follows our style guide and
passes our tests.
Update or add tests (if any source code was changed or added) 🧪
Update or add documentation (if features were changed or added) 📝
Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

ajschmidt8

Leaving some notes for additional context.

ajschmidt8 · 2023-04-23T22:41:09Z

src/commands/deploy/deploy.mjs

@@ -178,31 +178,6 @@ const validateFolders = async ({ deployFolder, functionsFolder }) => {
  return { deployFolderStat, functionsFolderStat }
 }

-const getDeployFilesFilter = ({ deployFolder, site }) => {


Unlike folder-walker, fast-glob does not accept a file filter function.

Therefore this function was deleted and its logic was adapted for the fast-glob file queries that exist in src/utils/deploy/hash-files.mjs.

ajschmidt8 · 2023-04-23T22:41:52Z

src/lib/edge-functions/deploy.mjs

@@ -8,17 +8,6 @@ import { EDGE_FUNCTIONS_FOLDER, PUBLIC_URL_PATH } from './consts.mjs'

 const distPath = getPathInProject([EDGE_FUNCTIONS_FOLDER])

-export const deployFileNormalizer = (rootDir, file) => {


This logic was moved to the normalizePath function in src/utils/deploy/util.mjs.

Skn0tt · 2023-04-24T09:13:33Z

Hi @ajschmidt8! I appreciate the PR. Properly supporting symlinks for netlify deploy is something we should fix! The PR is pretty big, so it'll take me a while to review it properly - please bear with me :)

Skn0tt · 2023-04-24T09:22:24Z

Looks like there's a linting error.

From a first cursory look, it's hard for me to differentiate the changes that fix the issue from refactorings. This makes it really hard for me to think about unwanted side-effects of this change. Would it be possible for you to trim this PR down to the minimal change that fixes the bug? I can also open a PR for that instead.

Again, i'm really happy you opened the PR, I'm just thinking that splitting things up into a fix PR and a refactoring PR would make it easier to review for me, and easier to merge with confidence.

ajschmidt8 · 2023-04-24T15:21:00Z

Looks like there's a linting error.

I'll resolve the linting errors and merge conflicts soon.

From a first cursory look, it's hard for me to differentiate the changes that fix the issue from refactorings. This makes it really hard for me to think about unwanted side-effects of this change. Would it be possible for you to trim this PR down to the minimal change that fixes the bug? I can also open a PR for that instead.

Again, i'm really happy you opened the PR, I'm just thinking that splitting things up into a fix PR and a refactoring PR would make it easier to review for me, and easier to merge with confidence.

I started down that route. Unfortunately most of these changes are intertwined. Here's why:

folder-walker only supports streams. fast-glob has APIs for streams, promises, and synchronous operations.

Since both libraries have stream support, you might think that they'd be swappable.

Unfortunately that's not the case since fast-glob requires two queries (vs. folder-walker's single query) to mimic the getDeployFilesFilter function that was used by folder-walker.

It seemed wasteful to devise a way to merge/concatenate these two independent fast-glob streams when the processing could be handled more succinctly using Promises. It would've introduced some throwaway work.

I realize there are a lot of changes. I'm happy to elaborate on any of them as needed.

I would recommend working backward and starting with reviewing the tests. If the test assertions looks sound, then you can be assured that the functions are working as expected.

Ultimately the majority of my changes were only made to hashFiles (hashFns was updated for consistency, but it's functionality didn't change). Any other file changes were only made because the hashFiles function interacted with them in some way.

During my development and debugging process, it was useful to set up a small demo site with symlinks to test out my changes.

I used npm link to switch back and forth between the main branch and my development branch and run netlify deploy on my demo site on each branch.

On each branch, I also added the statements below before the return statement in hashFiles to ensure that its return values were consistent with my expectations:

console.log("files:", files)
console.log("filesShaMap:", filesShaMap)
process.exit(0) // exit early to prevent having to wait for a full deploy to occur

ajschmidt8 · 2023-04-24T15:44:22Z

I can also open a PR for that instead.

I'm not opposed to you opening a separate PR.

But I'd prefer not to spend too much more time on these changes myself (aside from minor PR requests) based on the time it took to get these changes working so far.

ajschmidt8 · 2023-04-24T15:50:59Z

src/utils/deploy/hasher-segments.mjs

all of the functionality from these functions is replaced by the few lines of code in the mapper functions in src/utils/deploy/hash-fns.mjs and src/utils/deploy/hash-files.mjs

Skn0tt · 2023-05-03T11:06:59Z

Hey @ajschmidt8! Please excuse me for not replying earlier. We've discussed this quite a bit internally, and while we agree that ideally the behaviour around Symlinks in CLI/NF Build should be the same, we are wary of other customers relying on this "buggy" behaviour. So we'd like to take any changes slow, and this might take a while.

I'll go ahead and close this PR for now - we will come back to solving this in the future, but we'll drive the implementation so you don't have to do any more work on this.

If you rely on this fix, I think you should be able to use your forked version of the CLI, right?

ajschmidt8 · 2023-05-03T17:43:24Z

Hey @ajschmidt8! Please excuse me for not replying earlier. We've discussed this quite a bit internally, and while we agree that ideally the behaviour around Symlinks in CLI/NF Build should be the same, we are wary of other customers relying on this "buggy" behaviour. So we'd like to take any changes slow, and this might take a while.

I'll go ahead and close this PR for now - we will come back to solving this in the future, but we'll drive the implementation so you don't have to do any more work on this.

If you rely on this fix, I think you should be able to use your forked version of the CLI, right?

Appreciate the follow-up.

I do hope you guys address this quickly.

I'd prefer to abandon my fork ASAP in favor of an upstream fix.

Keep me posted if there's anything I can do to help with this effort.

CandiedCode · 2024-09-17T14:44:24Z

Please excuse me for not replying earlier. We've discussed this quite a bit internally, and while we agree that ideally the behaviour around Symlinks in CLI/NF Build should be the same, we are wary of other customers relying on this "buggy" behaviour. So we'd like to take any changes slow, and this might take a while.

@Skn0tt the way around this is to provide some type of flag that a user can set to follow a symlink. This is causing me issues right now as well, and for such an old issue has there been no update on providing this support today?

fix: follow symlinks w/ netlify deploy

8a07c3e

ajschmidt8 requested a review from a team April 23, 2023 22:39

ajschmidt8 added 2 commits April 23, 2023 18:44

chore: update tests

2eb46f0

chore: update dependencies

3f2ea28

ajschmidt8 force-pushed the fix-symlinks branch from fdde8b6 to 3f2ea28 Compare April 23, 2023 22:44

ajschmidt8 commented Apr 23, 2023

View reviewed changes

ajschmidt8 mentioned this pull request Apr 23, 2023

Deploy does not follow symlinks #1809

Open

ajschmidt8 added 3 commits April 24, 2023 11:35

fix: lint warning from unrelated file?

db902e4

Merge remote-tracking branch 'upstream/main' into fix-symlinks

4e5e23c

fix: prettier formatting

e8318e9

ajschmidt8 commented Apr 24, 2023

View reviewed changes

Skn0tt closed this May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: follow symlinks w/ `netlify deploy` #5650

fix: follow symlinks w/ `netlify deploy` #5650

ajschmidt8 commented Apr 23, 2023

ajschmidt8 left a comment

ajschmidt8 Apr 23, 2023 •

edited

Loading

ajschmidt8 Apr 23, 2023

Skn0tt commented Apr 24, 2023 •

edited

Loading

Skn0tt commented Apr 24, 2023 •

edited

Loading

ajschmidt8 commented Apr 24, 2023 •

edited

Loading

ajschmidt8 commented Apr 24, 2023

ajschmidt8 Apr 24, 2023

Skn0tt commented May 3, 2023

ajschmidt8 commented May 3, 2023

CandiedCode commented Sep 17, 2024 •

edited

Loading

		@@ -8,17 +8,6 @@ import { EDGE_FUNCTIONS_FOLDER, PUBLIC_URL_PATH } from './consts.mjs'

		const distPath = getPathInProject([EDGE_FUNCTIONS_FOLDER])

		export const deployFileNormalizer = (rootDir, file) => {

fix: follow symlinks w/ netlify deploy #5650

fix: follow symlinks w/ netlify deploy #5650

Conversation

ajschmidt8 commented Apr 23, 2023

Summary

ajschmidt8 left a comment

Choose a reason for hiding this comment

ajschmidt8 Apr 23, 2023 • edited Loading

Choose a reason for hiding this comment

ajschmidt8 Apr 23, 2023

Choose a reason for hiding this comment

Skn0tt commented Apr 24, 2023 • edited Loading

Skn0tt commented Apr 24, 2023 • edited Loading

ajschmidt8 commented Apr 24, 2023 • edited Loading

ajschmidt8 commented Apr 24, 2023

ajschmidt8 Apr 24, 2023

Choose a reason for hiding this comment

Skn0tt commented May 3, 2023

ajschmidt8 commented May 3, 2023

CandiedCode commented Sep 17, 2024 • edited Loading

fix: follow symlinks w/ `netlify deploy` #5650

fix: follow symlinks w/ `netlify deploy` #5650

ajschmidt8 Apr 23, 2023 •

edited

Loading

Skn0tt commented Apr 24, 2023 •

edited

Loading

Skn0tt commented Apr 24, 2023 •

edited

Loading

ajschmidt8 commented Apr 24, 2023 •

edited

Loading

CandiedCode commented Sep 17, 2024 •

edited

Loading