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(compiler-sfc): malformed filename on windows using path.posix.join() #9478

Merged
merged 8 commits into from
Nov 13, 2023
Merged

fix(compiler-sfc): malformed filename on windows using path.posix.join() #9478

merged 8 commits into from
Nov 13, 2023

Conversation

b12k
Copy link
Contributor

@b12k b12k commented Oct 25, 2023

Closes: #8671, #9583

Not fixed with: #9446

Related: #9473

On Windows path.posix is available but path.posix.join() returns malformed result when joining paths containing .. like ../folderOrFile

Error:
image

where ../base folder contains index.ts with "re-exports" like:

//...
export { default as BaseImage, type BaseImageProps } from './base-image.vue';
//...

Simple reproducible example (on Windows):

Save the following as index.js and run node index.js

const path = require('path');

const fileName = '../fileOrFolder';

console.log({
    join: path.join(__filename, fileName), // ✅ >> C:\\code\\sandbox\\posix-path\\fileOrFolder
    posixJoin: path.posix.join(__filename, fileName), // ❌ >> fileOrFolder
    resolve: path.resolve(__filename, fileName), // ✅ >> C:\\code\\sandbox\\posix-path\\fileOrFolder
    posixResolve: path.posix.resolve(__filename, fileName), // ✅ >> /code/sandbox/posix-path/fileOrFolder
})

Fix forces to use path.join() instead of path.posix.join() on Windows.

@b12k
Copy link
Contributor Author

b12k commented Oct 25, 2023

@sodatea I recreated this fix and added more details.

Thank you in advance!

FYI: @edison1105

@pikax
Copy link
Member

pikax commented Oct 26, 2023

@b12k can you provide an example of this error happening, I've been using windows and haven't noticed this issue.

@github-actions
Copy link

github-actions bot commented Oct 26, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 86.5 kB 32.9 kB 29.7 kB
vue.global.prod.js 132 kB 49.6 kB 44.5 kB

Usages

Name Size Gzip Brotli
createApp 48 kB 18.9 kB 17.2 kB
createSSRApp 51.2 kB 20.2 kB 18.4 kB
defineCustomElement 50.4 kB 19.7 kB 17.9 kB
overall 61.4 kB 23.7 kB 21.6 kB

@b12k
Copy link
Contributor Author

b12k commented Oct 26, 2023

resolveType.spec.ts

Test relative ts on Windows does not include condition when file is located in another folder.

I will update the test to illustrate the issue

Copy link
Member

@haoqunjiang haoqunjiang left a comment

Choose a reason for hiding this comment

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

joinPaths is also used in minimatch patterns:

included.some(p => isMatch(containingFile, joinPaths(base, p)))
which only accepts forward slashes: https://github.com/isaacs/minimatch#windows
I'm afraid that's what's causing the Windows test failures: https://github.com/vuejs/core/actions/runs/6651662225/job/18074220591?pr=9478

I recommend only modifying the code that resolves the relative path to avoid accidental breakages.

@b12k
Copy link
Contributor Author

b12k commented Oct 26, 2023

joinPaths is also used in minimatch patterns:

included.some(p => isMatch(containingFile, joinPaths(base, p)))

which only accepts forward slashes: https://github.com/isaacs/minimatch#windows
I'm afraid that's what's causing the Windows test failures: https://github.com/vuejs/core/actions/runs/6651662225/job/18074220591?pr=9478

I recommend only modifying the code that resolves the relative path to avoid accidental breakages.

  • Using path.join for Windows in importSourceToScope
  • Added respective test

@b12k b12k requested a review from haoqunjiang October 26, 2023 15:29
@b12k
Copy link
Contributor Author

b12k commented Oct 26, 2023

Strange that e2e failed.
It passes locally on Windows and Ubuntu
image

@b12k
Copy link
Contributor Author

b12k commented Oct 27, 2023

@sodatea May I please ask you to have a look one more time?

@b12k
Copy link
Contributor Author

b12k commented Oct 27, 2023

@b12k can you provide an example of this error happening, I've been using windows and haven't noticed this issue.

I created a test case for that
packages/compiler-sfc/tests/compileScript/resolveType.spec.ts

You may reproduce that by adding a test case like in PR and try running unit tests on Windows.

@b12k
Copy link
Contributor Author

b12k commented Oct 31, 2023

@sodatea I really need this fix =) We can not directly import types/interfaces, and using (redundant file) *.ts import/export workaround.

@yyx990803 yyx990803 added the ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. label Nov 13, 2023
@yyx990803 yyx990803 merged commit f18a174 into vuejs:main Nov 13, 2023
11 checks passed
@b12k b12k deleted the fix/malformed-filename-on-windows-with-path-posix-join branch November 13, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants