-
Notifications
You must be signed in to change notification settings - Fork 359
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
feat: Resolver control by environment variable "LLRT_PLATFORM" #690
feat: Resolver control by environment variable "LLRT_PLATFORM" #690
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! Just a minor suggestion
@richarddavison , We have also fixed the issue where Windows paths were not being processed correctly, so please check it out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can avoid a few unwraps. I wonder if we should do this on all paths on after calling require_resolve
instead of on a per function basis. Also make that function private to make sure it always return an abs path.
Thinking about it, in this use case, we only need to worry about the package.json resolution process, and since that function can return a Result type, we can avoid using unwrap(). |
Issue # (if available)
#680 (comment)
Description of changes
This PR adds the ability to toggle platform preferences in the Node.js package resolver.
Use "browser":
Use "node":
NOTE: This behavior is currently as expected, as the tty module has not yet been merged into mainline.
Non use:
Checklist
tests/unit
and/or in Rust for my feature if neededmake fix
to format JS and apply Clippy auto fixesmake check
types/
directoryBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.