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

Improve resolve on windows #689

Closed
wants to merge 1 commit into from
Closed

Conversation

ahaoboy
Copy link
Contributor

@ahaoboy ahaoboy commented Nov 18, 2024

Description of changes

The following test fails on Windows due to the path separator

  _require(`${CWD}/fixtures/test_modules/test-uuid.js`);

Checklist

  • Created unit tests in tests/unit and/or in Rust for my feature if needed
  • Ran make fix to format JS and apply Clippy auto fixes
  • Made sure my code didn't add any additional warnings: make check
  • Added relevant type info in types/ directory
  • Updated documentation if needed (API.md/README.md/Other)

Comment on lines 76 to 88
require_resolve(ctx, name, base, true).map(|name| {
if cfg!(not(windows)) {
name.into_owned()
} else {
name.replace("\\", "/")
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a "to_abs_path" used by require_resolve in various places. Use that method inside of require resolve where this case isn't covered!

@nabetti1720
Copy link
Contributor

nabetti1720 commented Nov 18, 2024

@ahaoboy , Thanks for addressing this issue. I may have missed it in the recent fixes.

The results returned by require_resolve() (build-in module name, bytecode cache name, absolute path including search results from package.json, presence or absence of prefix used on the loader side, etc.) vary, so I think it's difficult to judge here.

I'll take a look at this if you don't mind. :)

@ahaoboy
Copy link
Contributor Author

ahaoboy commented Nov 18, 2024

The results returned by require_resolve() (build-in module name, bytecode cache name, absolute path including search results from package.json, presence or absence of prefix used on the loader side, etc.) vary, so I think it's difficult to judge here.

I'll take a look at this if you don't mind. :)

The logic is indeed a bit complicated, and I'm not sure whether it should be handled here. If there is a better way, you can directly submit a new PR

@ahaoboy
Copy link
Contributor Author

ahaoboy commented Nov 18, 2024

Strange error
https://github.com/awslabs/llrt/actions/runs/11892672367/job/33136055896

And we may also need to handle node protocols such as node:fs

@nabetti1720
Copy link
Contributor

@ahaoboy , This issue was addressed in #690. We have confirmed that the Require tests pass at least in windows-build.

image

@nabetti1720
Copy link
Contributor

@ahaoboy, #690 has been merged into mainline, so this PR is no longer needed. :)

@ahaoboy ahaoboy closed this Nov 19, 2024
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