Skip to content

Conversation

@zhxiaoyu
Copy link

  1. Use commands through cmd.exe /C in Windows to ensures environment variables are loaded automatically
  2. Explicitly specify bash as shell for all runner because powershell doesn't support the && operator
  3. Handle the difference between "\n" and "\r\n" on Linux and Windows
  4. Handle the difference between path symbols on Linux and Windows

@Wulf
Copy link
Owner

Wulf commented Jul 27, 2025

@zhxiaoyu thank you very much for this. I'd like to ask you one last thing before we merge this in:

Does this solution work on windows?

If so, I think it can also help us avoid any required path.resolve(__dirname, ...) for end-users in their vite config files. If not, I guess we'll have to document this caveat for windows users.

@zhxiaoyu
Copy link
Author

@zhxiaoyu thank you very much for this. I'd like to ask you one last thing before we merge this in:

Does this solution work on windows?

If so, I think it can also help us avoid any required path.resolve(__dirname, ...) for end-users in their vite config files. If not, I guess we'll have to document this caveat for windows users.

Hi @Wulf , this works on windows by my test.

@thespooler
Copy link

Is there any plan to release this or does it need more work?

@Wulf
Copy link
Owner

Wulf commented Sep 17, 2025

Hey @thespooler,

It looks good to me, I was just hoping we could attempt something like this from the link above:

#[cfg(windows)]
pub const NPM: &'static str = "npm.cmd";

#[cfg(not(windows))]
pub const NPM: &'static str = "npm";

It’ll make the code much easier to read and maintain instead of having to branch so much. If someone can test this, it would be wonderful, especially for future devs jumping into the codebase. Otherwise, I might have to merge this in as-is.

Moreover:

it can also help us avoid any required path.resolve(__dirname, ...) for end-users in their vite config files. If not, I guess we'll have to document this caveat for windows users.

@zhxiaoyu
Copy link
Author

Hey @thespooler,

It looks good to me, I was just hoping we could attempt something like this from the link above:

#[cfg(windows)]
pub const NPM: &'static str = "npm.cmd";

#[cfg(not(windows))]
pub const NPM: &'static str = "npm";

It’ll make the code much easier to read and maintain instead of having to branch so much. If someone can test this, it would be wonderful, especially for future devs jumping into the codebase. Otherwise, I might have to merge this in as-is.

Moreover:

it can also help us avoid any required path.resolve(__dirname, ...) for end-users in their vite config files. If not, I guess we'll have to document this caveat for windows users.

Hi @Wulf , I have modified it as this method.

@thespooler
Copy link

I did try @zhxiaoyu branch and it does work on Windows, no issues. cargo test seems to fail for me tho.

@zhxiaoyu
Copy link
Author

I did try @zhxiaoyu branch and it does work on Windows, no issues. cargo test seems to fail for me tho.

Hi @thespooler , could you provide the detailed error information?

@thespooler
Copy link

It fails on normal_usage_test.

running 1 test
test test ... FAILED

failures:

---- test stdout ----

thread 'test' panicked at crates\vite-rs\tests\normal_usage_test.rs:82:9:
assertion `left == right` failed
  left: 491
 right: 489
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    test

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.09s

@zhxiaoyu
Copy link
Author

Have you installed npm packages for examples & tests like this?

(cd ./crates/vite-rs/test_projects && npm ci)
(cd ./crates/vite-rs-axum-0-8/test_projects/basic_usage_test/app && npm ci)
(cd ./crates/vite-rs-axum-0-8/test_projects/ctrl_c_handling_test/app && npm ci)
(cd ./crates/vite-rs-axum-0-8/examples/basic_usage/app && npm ci)
(cd ./crates/vite-rs/examples/vite-project-folder && npm ci)

@thespooler
Copy link

You got it... All good now, ✅ all tests pass.

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.

3 participants