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: Traverse symlink folders #122

Merged
merged 4 commits into from
Mar 24, 2024

Conversation

joshhunt
Copy link
Contributor

#118 introduced a slight regression where symlinked directories would not be traversed during walkdir. This PR checks if dirents are symlinks + and traverses them as appropriate.

Fixes gulpjs/vinyl-fs#349

Comment on lines +80 to +82
if (err) {
return cb(err);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure what to do about the error handling here. This cb tells the queue an error occured, but presumably dirents.forEach(processDirent); will continue processing, which could potentially error again...

Should it instead just throw the error and hope queue catches and emits it properly?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pointing this out! It has me rethinking how things are queued, but I'll make those changes after I land this

@phated phated self-assigned this Mar 23, 2024
Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

@joshhunt Thank you so much for this! You are a true hero in digging into the issue and supplying a fix. Please let me know if there is anything I can do to keep you actively involved in our community <3

@phated phated merged commit d49d9bd into gulpjs:master Mar 24, 2024
15 checks passed
@joshhunt
Copy link
Contributor Author

joshhunt commented Mar 25, 2024

Neat - thanks for the quick review and getting this one in @phated.

Do you think we could cut a release of vinyl-fs with this version? I'm happy to PR whatever's needed for that (just version bump?)

@phated
Copy link
Member

phated commented Mar 25, 2024

@joshhunt It's actually unnecessary to release a new vinyl-fs, as we use the semver caret (^) to indicate ^8.0.0 which will pick up this release automatically! 🎉 Thanks again for your help here.

@sunnysonx
Copy link

On WSL, it introduced a bug with maximum call stack on i18next-scanner package.

Despite the fact that I do not have symlinks inside my project, it reaches the maximum call stack.
image

I have downgraded glob-stream to 8.0.0 and it resolved the issue.

All our current projects that were using i18next-scanner started to fail if glob-stream was updated to 8.0.1

@joshhunt
Copy link
Contributor Author

@sunnysonx Please check in on the conversation in #125.

A reproduction, or some debugging to understand what path it is choking on would be appreciated 🙏

@sunnysonx
Copy link

@joshhunt I'm sorry, idk how it happened that I searched across all involved packages, but it seems that I ommited to search in glob-stream. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vfs.src glob does not match symlinks
3 participants