feat(env): apply initial use during env --use-on-cd (Schniz#1516) #183
Merged
Conversation
* fix(zsh): dedupe use-on-cd hook on re-source * fix(wincmd): support spaces and drive switches in use-on-cd * test(e2e): cover use-on-cd after re-sourcing env * chore: add changeset for use-on-cd shell fixes
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* feat: support x64-glibc-217 * add changeset --------- Co-authored-by: Gal Schlezinger <gal@spitfire.co.il>
* fix(use): clarify interactive install prompt source * chore: add changeset for prompt clarification
* perf(install): pin shell in generated env setup * ci: pin docker platform for ARM installer tests * chore: add changeset for installer shell update
|
@Schniz is attempting to deploy a commit to the Foundry development Team on Vercel. A member of the Team first needs to authorize it. |
Reviewer's GuideImplements immediate application of the current directory’s Node version when using Sequence diagram for fnm env --use-on-cd initial version applicationsequenceDiagram
actor User
participant Shell
participant EnvCommand
participant UseCommand
participant VersionFiles
User->>Shell: run fnm env --use-on-cd --shell bash
Shell->>EnvCommand: execute Env with use_on_cd=true
EnvCommand->>EnvCommand: set_path_for_multishell(multishell_path)
EnvCommand->>EnvCommand: config_with_multishell = config.clone().with_multishell_path(multishell_path)
EnvCommand->>UseCommand: apply(config_with_multishell)
UseCommand->>VersionFiles: read .node-version in current directory
VersionFiles-->>UseCommand: desired Node version
UseCommand->>UseCommand: switch Node version for multishell
UseCommand-->>EnvCommand: Ok(()) or Error(ignored)
EnvCommand->>Shell: print shell.use_on_cd(config) script
Shell-->>User: evaluates env output, hook installed and initial version already active
Sequence diagram for zsh use-on-cd hook de-duplicationsequenceDiagram
participant ZshShell
participant EnvCommand
ZshShell->>EnvCommand: fnm env --use-on-cd --shell zsh
EnvCommand-->>ZshShell: script with add-zsh-hook -D chpwd _fnm_autoload_hook
ZshShell->>ZshShell: evaluate script, remove existing _fnm_autoload_hook
ZshShell->>ZshShell: add-zsh-hook chpwd _fnm_autoload_hook
ZshShell->>EnvCommand: fnm env --use-on-cd --shell zsh (sourced again)
EnvCommand-->>ZshShell: same script
ZshShell->>ZshShell: add-zsh-hook -D chpwd _fnm_autoload_hook (de-duplicate)
ZshShell->>ZshShell: add-zsh-hook chpwd _fnm_autoload_hook (single active hook)
Updated class diagram for Use, FnmConfig, Arch, and DirectoriesclassDiagram
class Use {
+Option~UserVersion~ version
+bool install_if_missing
+bool silent_if_unchanged
+bool info_to_stderr
+apply(config: FnmConfig) Result~(), Error~
}
class FnmConfig {
+Url node_dist_mirror
+Option~PathBuf~ base_dir
+Option~PathBuf~ multishell_path
+with_base_dir(base_dir: Option~PathBuf~) FnmConfig
+with_multishell_path(multishell_path: PathBuf) FnmConfig
}
class Arch {
<<enum>>
X86
X64
X64Musl
X64Glibc217
Arm64
Armv7l
Ppc64le
Aarch64AppleDarwin
+as_str() &str
+from_str(s: &str) Result~Arch, Error~
}
class Directories {
<<struct>>
+cache_dir: PathBuf
+data_dir: PathBuf
+config_dir: PathBuf
+new() Directories
}
Use --> FnmConfig : uses
FnmConfig --> Directories : contains
FnmConfig --> Arch : uses for downloads
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Owner
Author
|
@Mergifyio refresh |
1 similar comment
Owner
Author
|
@Mergifyio refresh |
Owner
Author
|
@Mergifyio update |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
☑️ Command
|
✅ Pull request refreshed |
☑️ Nothing to do, the required conditions are not metDetails
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
set_path_for_multishell, theunsafeblock aroundstd::env::set_varis unnecessary and could be removed to avoid implying special invariants where none are required. set_path_for_multishellalways unconditionally prepends the multishell path toPATH; if this function is ever called more than once in a process, it will grow duplicate entries, so consider checking for and avoiding re-adding an existing entry.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `set_path_for_multishell`, the `unsafe` block around `std::env::set_var` is unnecessary and could be removed to avoid implying special invariants where none are required.
- `set_path_for_multishell` always unconditionally prepends the multishell path to `PATH`; if this function is ever called more than once in a process, it will grow duplicate entries, so consider checking for and avoiding re-adding an existing entry.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces a significant performance optimization for fnm env --use-on-cd by applying the initial Node.js version directly within the env command, which avoids spawning an extra fnm use subprocess. The changes also include support for the x64-glibc-217 architecture, several fixes for shell hooks in zsh and Windows CMD to improve robustness, and corresponding updates to documentation and tests. Overall, the changes are well-structured and beneficial. However, I have a significant concern regarding the use of an unsafe block to modify the PATH environment variable, as this is not thread-safe and could lead to undefined behavior in a multi-threaded context. I've left a specific comment detailing this issue and suggesting a safer alternative.
Owner
Author
|
@Mergifyio refresh |
✅ Pull request refreshed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Apply the initial Node version when generating
fnm env --use-on-cdto reduce shell startup overhead and improve hook behavior across shells, while adding new architecture support and updating installer, docs, and tests for the new behavior.New Features:
fnm env --use-on-cd, eliminating the need for an immediate extrafnm usesubprocess on shell startup.x64-glibc-217architecture via the--arch x64-glibc-217option infnm env.Bug Fixes:
--use-on-cdhook is de-duplicated on re-sourcing the shell configuration.--use-on-cdbehavior by quoting the cd macro path, handling paths with spaces, and switching drives withcd /d.--platformsetting.Enhancements:
fnm usethrough stderr when invoked byfnm envto avoid interfering with env output.fnm usemissing-version prompt by prefixing it withfnmso users can see which tool is requesting installation.--shellflags in generated shell setup and installer scripts to avoid runtime shell inference and speed up startup.--use-on-cdhooks so they no longer run the autoload hook immediately, relying instead on cd-triggered execution.Build:
CI:
--platformwith Docker for ARM images and normalize YAML quoting/style.Documentation:
--shellflags tofnm envin README and command docs, and fix minor README formatting issues.env --use-on-cdimprovements and new CLI features.Tests:
env --use-on-cdimmediately honors the current directory version and still works after sourcing env multiple times.use-on-cdmacro correctly quotes paths with spaces.Chores:
x64-glibc-217arch support, installer shell flag preference, and the clarified missing-version prompt.