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

Result paths are not normalized to platform #370

Closed
CITguy opened this issue Aug 16, 2022 · 4 comments
Closed

Result paths are not normalized to platform #370

CITguy opened this issue Aug 16, 2022 · 4 comments

Comments

@CITguy
Copy link

CITguy commented Aug 16, 2022

Environment

  • OS Version: Windows 10
  • Node.js Version: 14.20.0

Actual behavior

Results from fast-glob use POSIX path separators, even when on Windows.

SRC C:\Users\JohnDoe\src
DEST C:\Users\JohnDoe\dist
SRC files (relative) [ 'secret.txt', 'foo/test.txt', 'foo/bar/fizz.txt' ]
SRC files (absolute) [ 
  'C:/Users/JohnDoe/src/secret.txt', 
  'C:/Users/JohnDoe/src/foo/test.txt', 
  'C:/Users/JohnDoe/src/foo/bar/fizz.txt' 
]
DEST files (relative) [ 
  'C:\\Users\\JohnDoe\\dist\\secret.txt', 
  'C:\\Users\\JohnDoe\\dist\\foo\\test.txt',
  'C:\\Users\\JohnDoe\\dist\\foo\\bar\\fizz.txt'
]
DEST files (absolute) [ 
  'C:\\Users\\JohnDoe\\src\\secret.txt', 
  'C:\\Users\\JohnDoe\\src\\foo\\test.txt', 
  'C:\\Users\\JohnDoe\\src\\foo\\bar\\fizz.txt' 
]

NOTE: DEST files (absolute) are all incorrect (still pointing to the SRC files), because string substitution failed to find C:\\Users\\JohnDoe\\src in the paths returned from fast-glob.

Expected behavior

I'd expect resulting file paths to be normalized (adhering to the platform-defined path separator).

SRC C:\Users\JohnDoe\src
DEST C:\Users\JohnDoe\dist
SRC files (relative) [ 'secret.txt', 'foo/test.txt', 'foo/bar/fizz.txt' ]
SRC files (absolute) [ 
  'C:\\Users\\JohnDoe\\src\\secret.txt', 
  'C:\\Users\\JohnDoe\\src\\foo\\test.txt',
  'C:\\Users\\JohnDoe\\src\\foo\\bar\\fizz.txt'
]
DEST files (relative) [ 
  'C:\\Users\\JohnDoe\\dist\\secret.txt', 
  'C:\\Users\\JohnDoe\\dist\\foo\\test.txt',
  'C:\\Users\\JohnDoe\\dist\\foo\\bar\\fizz.txt'
]
DEST files (absolute) [ 
  'C:\\Users\\JohnDoe\\dist\\secret.txt', 
  'C:\\Users\\JohnDoe\\dist\\foo\\test.txt',
  'C:\\Users\\JohnDoe\\dist\\foo\\bar\\fizz.txt'
]

Steps to reproduce

  1. cd C:\Users\JohnDoe
  2. node copyFiles.mjs

Code sample

Given the following directory structure of C:\Users\JohnDoe:

src/
  secret.txt
  foo/
    test.txt
    bar/
      fizz.txt

C:\Users\JohnDoe\copyFiles.mjs

import fg from 'fast-glob'
import path from 'path'

const SRC = path.resolve('src')
const DEST = path.resolve('dist')

async function main() {
  console.log('SRC', SRC)
  console.log('DEST', DEST)

  // fetch source paths
  let relFiles = await fg([ '**/*.txt' ], { cwd: SRC })
  let absFiles = await fg([ '**/*.txt' ], { cwd: SRC, absolute: true })
  console.log('SRC files (relative)', relFiles)
  console.log('SRC files (absolute)', absFiles)

  // compute destination paths
  let relFilesDest = relFiles.map(relFile => path.resolve(DEST, relFile))
  let absFilesDest = absFiles.map(absFile => absFile.replace(SRC, DEST))
  console.log('DEST files (relative)', relFilesDest)
  console.log('DEST files (absolute)', absFilesDest)
  
  // perform "copy" logic here...
}

main()

EDIT: 2022-08-23 - expanded example, expected results, and actual results to reflect { absolute: true } config

@mrmlnc
Copy link
Owner

mrmlnc commented Aug 22, 2022

Works as expected. Users write patterns in POSIX-style. The result of the work is also presented in this style.

The Node.js platform can work with POSIX-style paths even on Windows.

I am ready to consider this functionality only if there are strong arguments.

@CITguy
Copy link
Author

CITguy commented Aug 23, 2022

The problem is more obvious when you use { absolute: true } in fast glob configs (updated issue contents, to reflect). The absolute path strings returned from fast-glob do not match the format returned by functions in the Node path module.

To perform string substitution on an absolute path returned from fast-glob, I have to normalize the fast-glob result first (so that it matches the format returned by path functions) and then proceed with the string substitution.

While path normalization is not a difficult task to accomplish, my expectation was that fast-glob would match the native path format when constructing absolute paths using path separators that match path.sep ("\\" on win32, "/" on POSIX).


If changes in this issue were to be made:

  1. POSIX environments won't see any change in the output for paths (regardless if they're relative or absolute)
  2. win32 environments that consume the absolute paths as-is, shouldn't be affected, because the paths are still valid in that environment
  3. 3rd-party logic on win32 environments that normalize the results wouldn't be affected, because the paths would already be normalized
  4. 3rd-party logic expecting absolute paths using the current format would either need to update their code to use the normalized paths OR fast-glob could provide an option to revert to the old behavior (which could then potentially be removed in a future release of fast-glob).

@mrmlnc
Copy link
Owner

mrmlnc commented Jul 24, 2023

I agree that when using the absolute option, we should return the path according to the OS used. Will be shipped with 4.0.0.

In the case of relative paths, things are more complicated, since we use the @nodelib/fs.walk package, which returns paths in POSIX format. I'll see what we can do about that. Perhaps we can use path from fs.Dirent (nodelib/nodelib#100).

@mrmlnc
Copy link
Owner

mrmlnc commented Aug 4, 2023

Will be included in the 4.0.0 release (#371).

Now, when the absolute option is enabled, the path is returned in the OS style.

@mrmlnc mrmlnc closed this as completed Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants