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(cli): normalize provided config path to satisfy esm-loader in windows #658

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

woai3c
Copy link
Contributor

@woai3c woai3c commented Mar 25, 2024

Description

resolved issue with ESM loader on Windows by converting file paths to file:// URLs

before

image

now

image

Test Plan


Copy link

netlify bot commented Mar 25, 2024

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 53302a6
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/66027459b9b0a3000812b6b1

@hyf0
Copy link
Member

hyf0 commented Mar 25, 2024

Good catch. Could you add a test for this case?

@woai3c
Copy link
Contributor Author

woai3c commented Mar 26, 2024

Good catch. Could you add a test for this case?

This seems difficult to write tests for because it is not like a function or a feature where you can control the inputs and outputs; changing the input of the function would change the output. The error is that it can't recognize the path, you need to use nodejs's pathToFileURL() to convert it into a path that is compatible and recognizable across different operating systems.

I tried writing some test cases to see what the test cases look like before the code is modified. It turns out that running the tests works fine, but directly executing the rolldown --config ./rolldown.config.js build task results in an error. Since I don't know much about vitest, I'm not sure if there is special handling inside, or if it is caused by other circumstances.

image

image

@woai3c
Copy link
Contributor Author

woai3c commented Mar 26, 2024

Good catch. Could you add a test for this case?

pathToFileURL() This function converts a file path into a file:// URL, which is valid on all platforms. On Linux and macOS, the file path is already a valid file:// URL, so the pathToFileURL function doesn't actually change the path. However, on Windows, because the path format is different from the file:// URL, the pathToFileURL function converts the path into a valid file:// URL.

import { pathToFileURL } from 'url'
import path from 'path'

// 在 Linux 和 macOS 上,这将输出 'file:///path/to/file'
// 在 Windows 上,这将输出 'file:///C:/path/to/file'
console.log(pathToFileURL(path.resolve('/path/to/file')))

@hyf0 hyf0 changed the title fix: resolved issue with ESM loader on Windows by converting file paths to file:// URLs fix(cli): normalize provided config path to satisfy esm-loader in windows Mar 26, 2024
@hyf0 hyf0 enabled auto-merge (squash) March 26, 2024 07:11
Copy link
Member

@hyf0 hyf0 left a comment

Choose a reason for hiding this comment

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

Thank you!

@hyf0 hyf0 merged commit be0540d into rolldown:main Mar 26, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants