fix(cli): handle patterns correctly on Windows #10430
Conversation
|
Can you please update the unit test as well? |
|
Sure, I'm working on that. |
fky2015
force-pushed
the
main
branch
3 times, most recently
from
2e88a70 to
ae4cb06
Compare
fky2015
changed the title
Modify the handling of patterns in the `crawl` function to correctly convert the current path to a pattern when it contains backslash on Windows, in according to fast-glob's docs.
|
@bo0tzz Can you review this PR for me? Thanks! |
| const convertPathToPatternOnWin = (path: string) => { | ||
| return platform() === 'win32' ? convertPathToPattern(path) : path; | ||
| }; |
There was a problem hiding this comment.
Does this need to be conditional to the platform, would there be any issues with just always converting?
There was a problem hiding this comment.
Yes, convertPathToPattern will change the matching behavior on *nix platform:
/photo* will become /photo\* thus the asterisk will lose its effect.
There was a problem hiding this comment.
But it doesn't change the behaviour on windows? If so that sounds weird, do you know why it is like that?
There was a problem hiding this comment.
No, it doesn't change the behaviour on Windows.
From my understanding, convertPathToPattern is a function especially used for converting a Windows path to fast-glob search pattern. It is meaningless to use this function if your inputs are a valid POSIX path.
| @@ -2,6 +2,7 @@ import { defineConfig } from 'vite'; | |||
| import tsconfigPaths from 'vite-tsconfig-paths'; | |||
|
|
|||
| export default defineConfig({ | |||
| resolve: { alias: { src: '/src' } }, | |||
There was a problem hiding this comment.
I followed cli/README.md and filed to build the project without this change to the config (on Windows).
error during build:
[vite]: Rollup failed to resolve import "src/commands/asset" from "D:\code\immich\cli\src\index.ts".
Generally speaking, I think this change will improve the compatibility of dev configs.
There was a problem hiding this comment.
There's a few changes in this test that look odd to me, can you explain what's going on here?
There was a problem hiding this comment.
Line #265: On the Windows platform, / cannot be treated as expected in fast-glob. I've filed an issue here mrmlnc/fast-glob#447.
Line #286 & #294: Comparing path strings directly will lead to some false positive cases. For example, on Windows, a path can be written as D:\Something\Like\This while \something\like\this on Linux. So It should be better to compare what they've actually read in the file system. To achieve this, I'm letting each file contain unique contents (their paths) so that it can be used for comparison later. A good thing to note is that we are still comparing their paths and this is friendly when debugging.
fast-globrequires slash (instead of backslash) for matching path.So before this change,
--recursivedidn't work on WIndows.Before:
After:
Some changes
convertPathToPatternfromfast-glob.