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: support symlinks without real paths with relative paths enabled #114

Conversation

SuperchupuDev
Copy link
Contributor

@SuperchupuDev SuperchupuDev commented Aug 27, 2024

fixes #113

when investigating, i found out that the issue was caused by the following line:

fdir/src/api/walker.ts

Lines 120 to 121 in 5a0aab5

let path = this.joinPath(entry.name, directoryPath);
this.resolveSymlink!(path, this.state, (stat, resolvedPath) => {

with relative paths on, path won't be an absolute path, and as such will be resolved in the stat call as a path relative to process.cwd(). this makes it throw an ENOENT error (unless the symlink happens to be located in process.cwd()). i fixed this by changing the declaration to make it an absolute path instead.

just fixing this line wasn't enough though, because the resolvedPath seems to always be absolute. to fix this, i did a substring transformation to a relative path (if the symlink is a file) if the relativePaths option is enabled and useRealPaths is false. the latter is because symlinks can point anywhere in the filesystem, so it should return an absolute path when that is the case.

directory symlinks with real paths sadly still do not work with relativePaths on. my plan was to make it return those as absolute paths, but that is impossible as the joinPath function is already built as the relative path implementation

since symlinks with real paths and relative paths still behave inconsistently before and after this pr, i marked in the docs that resolving symlinks with real paths is only supported if the resolveSymlinks argument is set to false

also, while working on this i found (and fixed) another bug, it looks like file symlinks were never normalized, and as such could have the wrong path separator etc. fixed this by moving the normalizePath call before the stat.isDirectory() check

@SuperchupuDev SuperchupuDev force-pushed the fix/partially-support-symlinks-with-relative-paths branch from fe02c4a to 96bdb71 Compare August 27, 2024 11:47
@SuperchupuDev
Copy link
Contributor Author

hi @thecodrr any chance you can take a look at this and #115 anytime soon? tinyglobby support for symlinks depends on it

src/api/walker.ts Outdated Show resolved Hide resolved
src/api/walker.ts Outdated Show resolved Hide resolved
src/api/walker.ts Outdated Show resolved Hide resolved
documentation.md Show resolved Hide resolved
__tests__/fdir.test.ts Show resolved Hide resolved
@SuperchupuDev SuperchupuDev force-pushed the fix/partially-support-symlinks-with-relative-paths branch from 96bdb71 to 1e278f9 Compare September 26, 2024 12:55
@SuperchupuDev SuperchupuDev force-pushed the fix/partially-support-symlinks-with-relative-paths branch from 73c0bfa to 91672cc Compare September 26, 2024 13:21
@thecodrr thecodrr merged commit d91cda2 into thecodrr:master Sep 26, 2024
11 checks passed
@SuperchupuDev SuperchupuDev deleted the fix/partially-support-symlinks-with-relative-paths branch September 26, 2024 14:58
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.

withSymlinks doesn't work properly with relative paths on when the symlink is at the top level
2 participants