Skip to content

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

Merged
yyx990803 merged 8 commits intovuejs:mainfrom
b12k:fix/malformed-filename-on-windows-with-path-posix-join
Nov 13, 2023
Merged

fix(compiler-sfc): malformed filename on windows using path.posix.join()#9478
yyx990803 merged 8 commits intovuejs:mainfrom
b12k:fix/malformed-filename-on-windows-with-path-posix-join

Conversation

@b12k
Copy link
Copy Markdown
Contributor

@b12k b12k commented Oct 25, 2023 •
edited
Loading

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 b12k mentioned this pull request Oct 25, 2023
Closed
@b12k
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

github-actions bot commented Oct 26, 2023 •
edited
Loading

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
Copy Markdown
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

haoqunjiang
haoqunjiang suggested changes Oct 26, 2023
View reviewed changes
Copy link
Copy Markdown
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:

core/packages/compiler-sfc/src/script/resolveType.ts

Line 883 in edf2572

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
Copy Markdown
Contributor Author

b12k commented Oct 26, 2023

joinPaths is also used in minimatch patterns:

core/packages/compiler-sfc/src/script/resolveType.ts

Line 883 in edf2572

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
Copy Markdown
Contributor Author

b12k commented Oct 26, 2023

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

@b12k
Copy link
Copy Markdown
Contributor Author

b12k commented Oct 27, 2023 •
edited
Loading

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

@b12k
Copy link
Copy Markdown
Contributor Author

b12k commented Oct 27, 2023 •
edited
Loading

@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.

haoqunjiang
haoqunjiang reviewed Oct 30, 2023
View reviewed changes
@b12k
Copy link
Copy Markdown
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.

@b12k b12k mentioned this pull request Nov 10, 2023
Closed
@b12k b12k requested a review from haoqunjiang November 10, 2023 23:56
@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
@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

Reviewers

1 more reviewer

@haoqunjiang haoqunjiang haoqunjiang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@b12k @pikax @haoqunjiang @yyx990803