resolve currentPath

[cleverpp]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

• resolves Build output is different when building on Windows or Mac #4496

Description

[lukastaegert]

Can you add a test that is fixed by this change?

[codecov]

Codecov Report

Merging #4591 (b6281b5) into master (fc08bcc) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4591   +/-   ##
=======================================
  Coverage   98.86%   98.86%           
=======================================
  Files         209      209           
  Lines        7345     7345           
  Branches     2098     2098           
=======================================
  Hits         7262     7262           
  Misses         27       27           
  Partials       56       56           

Impacted Files	Coverage Δ
src/Chunk.ts	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[cleverpp]

Can you add a test that is fixed by this change?

@lukastaegert ，you mean in test directory add a new test case？

i had found a test case “test/chunking-form/samples/preserve-modules-root “， it do pass in windows。

should i need add a new test case？

[lukastaegert]

Yes. The old test case was green without your change, so it was not good enough. We need a test that does not pass without your change. It would also help me and others understand the issue better, is useful for debugging and of course prevents us from breaking it again if we change the logic later

[Tanimodori]

I've digged more deeply into this issue that it turns out to be an issue with vite's id normalization.

TL;DR Vite normalize all file ids to use slash / on all platforms, but rollup uses path.resolve which is platform-dependent (backslash \\ for Windows). Which causes this problem.

vite side

There are functions defined in vite/packages/vite/src/node/utils.ts to normalize slash, namely slash, normalizePath, fsPathFromId.

export function slash(p: string): string {
  return p.replace(/\\/g, '/')
}

export function normalizePath(id: string): string {
  return path.posix.normalize(isWindows ? slash(id) : id)
}

export function fsPathFromId(id: string): string {
  const fsPath = normalizePath(
    id.startsWith(FS_PREFIX) ? id.slice(FS_PREFIX.length) : id
  )
  return fsPath.startsWith('/') || fsPath.match(VOLUME_RE)
    ? fsPath
    : `/${fsPath}`
}

The vite:resolve plugin is used to resolve id, which get normalized and sent to rollup.

      // drive relative fs paths (only windows)
      if (isWindows && id.startsWith('/')) {
        const basedir = importer ? path.dirname(importer) : process.cwd()
        const fsPath = path.resolve(basedir, id)
        if ((res = tryFsResolve(fsPath, options))) {
          isDebug &&
            debug(`[drive-relative] ${colors.cyan(id)} -> ${colors.dim(res)}`)
          return res
        }
      } 
      
      // absolute fs paths
      if (
        isNonDriveRelativeAbsolutePath(id) &&
        (res = tryFsResolve(id, options))
      ) {
        isDebug && debug(`[fs] ${colors.cyan(id)} -> ${colors.dim(res)}`)
        return res
      }

The tryFsResolve calls tryResolveFile, getRealPath and finally normalizePath. I'll omit those code, you get the idea. They are just wrappers.

The overall vite repository is written in the assumption that path sep is slash. There are tons of hard coded slash over its repository.

rollup side

The test can pass if rollup handles ids by itself. Recall that the condition is:

const currentDir = dirname(sanitizedId);
// ...
const currentPath = `${currentDir}/${fileName}`;
// ...
if (preserveModulesRoot && currentPath.startsWith(preserveModulesRoot)) {
  // ...
}

On Windows that will turns out like

Situation	currentPath	preserveModulesRoot	Outcome
pure rollup build	C:\\path/foo.ext	C:\\path	true
vite build	C:/path/foo.ext	C:\\path	false

And the node path.dirname preserves slashes.

console.log(path.dirname('C:\\foo\\bar.ext')); // -> 'C:\\foo'
console.log(path.dirname('C:\\foo/bar.ext'));  // -> 'C:\\foo'
console.log(path.dirname('C:/foo/bar.ext'));   // -> 'C:/foo'

So the old test is still valid, as long as ids are not passed from outside.

Conclusion

I'm afraid adding a resolve to currentPath is the only fix, since it will take significant time to rewrite vite for slash problems.

[Tanimodori]

Related issues: #4496 vitejs/vite#6509

[lukastaegert]

I added the test myself...