feat(env): apply initial use during env --use-on-cd#207
Open
Dargon789 wants to merge 6 commits intoDargon789:revert-118-revert-115-Dargon789-patch-9from
Open
feat(env): apply initial use during env --use-on-cd#207Dargon789 wants to merge 6 commits intoDargon789:revert-118-revert-115-Dargon789-patch-9from
Dargon789 wants to merge 6 commits intoDargon789:revert-118-revert-115-Dargon789-patch-9from
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
Reviewer's GuideImplements eager application of the initial Node version during Sequence diagram for fnm env --use-on-cd eager initial usesequenceDiagram
actor User
participant Shell
participant FnM_env as FnM_env_command
participant UseCmd as Use_command
User->>Shell: start shell session
Shell->>FnM_env: run fnm env --use-on-cd --shell <shell>
FnM_env->>FnM_env: compute multishell_path
FnM_env->>FnM_env: set_path_for_multishell(multishell_path)
FnM_env->>FnM_env: config_with_multishell = config.clone().with_multishell_path(multishell_path)
FnM_env->>FnM_env: construct Use{version=None, install_if_missing=false, silent_if_unchanged=true, info_to_stderr=true}
FnM_env->>FnM_env: determine should_force_stderr_color
alt should_force_stderr_color
FnM_env->>FnM_env: colored::control::set_override(true)
end
FnM_env->>UseCmd: apply(config_with_multishell)
Note over UseCmd: Reads version file in current directory
UseCmd-->>FnM_env: Result (ignored on error)
alt should_force_stderr_color
FnM_env->>FnM_env: colored::control::unset_override()
end
FnM_env-->>Shell: shell.use_on_cd(config) script
Shell->>Shell: install cd hook using returned script
Shell-->>User: environment ready, directory version already active
Class diagram for updated Use, FnmConfig, Directories, and ArchclassDiagram
class Command{
<<interface>>
+apply(config: FnmConfig) Result
}
class Use{
+Option~UserVersionReader~ version
+bool install_if_missing
+bool silent_if_unchanged
+bool info_to_stderr
+apply(config: FnmConfig) Result
}
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 Directories{
<<struct>>
<<Clone>>
+cache_dir() PathBuf
+node_dist_mirror_dir() PathBuf
}
class Arch{
<<enum>>
X86
X64
X64Musl
X64Glibc217
Arm64
Armv7l
Ppc64le
S390x
+as_str() &str
+from_str(s: &str) Result~Arch~
}
Use ..|> Command
FnmConfig o-- Directories
FnmConfig .. Arch
Use ..> FnmConfig
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 update |
☑️ Nothing to do, the required conditions are not metDetails
|
Owner
Author
Owner
Author
|
@Mergifyio refresh |
Owner
Author
|
@Mergifyio rebase |
✅ Pull request refreshed |
☑️ Nothing to do, the required conditions are not metDetails
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
set_path_for_multishell, theunsafeblock aroundstd::env::set_varis unnecessary sinceset_varis safe; you can drop theunsafeto simplify the function. - When prepending
path_for_nodetoPATHinset_path_for_multishell, consider skipping insertion if that path is already present to avoid unbounded PATH growth whenfnm env --use-on-cdis evaluated multiple times.
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 since `set_var` is safe; you can drop the `unsafe` to simplify the function.
- When prepending `path_for_node` to `PATH` in `set_path_for_multishell`, consider skipping insertion if that path is already present to avoid unbounded PATH growth when `fnm env --use-on-cd` is evaluated multiple times.
## Individual Comments
### Comment 1
<location path="src/cli.rs" line_range="30" />
<code_context>
/// should be evaluated by your shell to create a fnm-ready environment.
///
/// Each shell has its own syntax of evaluating a dynamic expression.
- /// For example, evaluating fnm on Bash and Zsh would look like `eval "$(fnm env)"`.
- /// In Fish, evaluating would look like `fnm env | source`
+ /// For example, evaluating fnm on Bash and Zsh would look like `eval "$(fnm env --shell bash)"`.
+ /// In Fish, evaluating would look like `fnm env --shell fish | source`
#[clap(name = "env", bin_name = "env")]
</code_context>
<issue_to_address>
**suggestion:** The example now uses `--shell bash` for both Bash and Zsh, which might be misleading for Zsh users.
Because `fnm env` accepts an explicit `--shell` value, Zsh users may reasonably expect to use `--shell zsh`. Showing `--shell bash` for Zsh as well can be confusing, even if it works. Please either add separate examples for Bash and Zsh or clarify in the text which `--shell` value each shell should use.
</issue_to_address>
### Comment 2
<location path=".changeset/brave-timers-move.md" line_range="5" />
<code_context>
+"fnm": patch
+---
+
+support `x64-glibc-217` arch by adding a `--arch x64-glibc-217` to fnm env
</code_context>
<issue_to_address>
**suggestion (typo):** Tighten up the grammar by adding an article before "arch" and adjusting the wording around the flag.
Consider rephrasing to: "Support the `x64-glibc-217` arch by adding `--arch x64-glibc-217` to `fnm env`." This fixes the missing article, removes the extra "a" before the flag, and formats command and flag names consistently with backticks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR updates fnm to v1.39.0, introducing x64-glibc-217 support and optimizing shell startup by applying the initial Node version during 'fnm env' to eliminate extra subprocesses. It also improves shell setup scripts and fixes hook issues in Zsh and Windows CMD. Feedback recommends replacing 'unsafe' environment modifications in 'src/commands/env.rs' with a 'skip_path_check' flag in the 'Use' command to safely suppress PATH validation during internal calls.
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.
feat(env): apply initial use during env --use-on-cd (https://github.com/Schniz/fnm/pull/1516[)](https://github.com/Dargon789/fnm/commit/10c68cb6630be811799afdccf212a399e08bef99)
@Schniz
Schniz authored on Mar 6
Summary by Sourcery
Apply initial Node version when using
fnm env --use-on-cd, improve shell hooks and installer guidance, and add support for thex64-glibc-217architecture.New Features:
fnm env --use-on-cdinstead of requiring an extrafnm usecall.x64-glibc-217architecture via the--arch x64-glibc-217flag infnm env.Bug Fixes:
--use-on-cdhooks robust for paths with spaces and drive changes.--use-on-cdhook de-duplicates existing hooks on re-source.fnm useexecution on shell init for fish, bash, and PowerShell hooks.Enhancements:
fnm useoutput to stderr when invoked internally byfnm envto keep stdout clean for shell evaluation.useexecution.Build:
CI:
Documentation:
--shellflags tofnm envin docs and installer output for better performance and reliability, and document updated usage examples for each shell.Tests:
env --use-on-cdand for re-sourcing behavior, plus unit tests for zsh and Windows CMD hook generation.Chores: