-
Notifications
You must be signed in to change notification settings - Fork 571
feat: enable --resolve-engines by default. out of experimental phase! #1265
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
--- | ||
"fnm": minor | ||
--- | ||
|
||
enable `--resolve-engines` by default. out of experimental phase. | ||
|
||
to disable it, add a `--resolve-engines=false` flag, and make sure to open an issue describing _why_. | ||
It might feel like a breaking change but .nvmrc and .node-version have precedence so it should not. | ||
|
||
I am all in favor of better experience and I believe supporting engines.node is a good direction. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ concurrency: | |
cancel-in-progress: true | ||
|
||
env: | ||
RUST_VERSION: "1.78" | ||
RUST_VERSION: "1.81" | ||
|
||
jobs: | ||
fmt: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
[toolchain] | ||
channel = "1.78" | ||
channel = "1.81" | ||
components = ["rustfmt", "clippy"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,10 @@ pub struct LsRemote { | |
|
||
/// Show only LTS versions (optionally filter by LTS codename) | ||
#[arg(long)] | ||
#[allow(clippy::option_option)] | ||
#[expect( | ||
clippy::option_option, | ||
reason = "clap Option<Option<T>> supports --x and --x=value syntaxes" | ||
)] | ||
Comment on lines
+16
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related? |
||
lts: Option<Option<String>>, | ||
|
||
/// Version sorting order | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,16 +77,24 @@ pub struct FnmConfig { | |
corepack_enabled: bool, | ||
|
||
/// Resolve `engines.node` field in `package.json` whenever a `.node-version` or `.nvmrc` file is not present. | ||
/// Experimental: This feature is subject to change. | ||
/// This feature is enabled by default. To disable it, provide `--resolve-engines=false`. | ||
/// | ||
/// Note: `engines.node` can be any semver range, with the latest satisfying version being resolved. | ||
/// Note 2: If you disable it, please open an issue on GitHub describing _why_ you disabled it. | ||
/// In the future, disabling it might be a no-op, so it's worth knowing any reason to | ||
/// do that. | ||
#[clap( | ||
long, | ||
env = "FNM_RESOLVE_ENGINES", | ||
global = true, | ||
hide_env_values = true, | ||
verbatim_doc_comment | ||
)] | ||
resolve_engines: bool, | ||
#[expect( | ||
clippy::option_option, | ||
reason = "clap Option<Option<T>> supports --x and --x=value syntaxes" | ||
)] | ||
resolve_engines: Option<Option<bool>>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ugh So |
||
|
||
#[clap(skip)] | ||
directories: Directories, | ||
|
@@ -102,7 +110,7 @@ impl Default for FnmConfig { | |
arch: Arch::default(), | ||
version_file_strategy: VersionFileStrategy::default(), | ||
corepack_enabled: false, | ||
resolve_engines: false, | ||
resolve_engines: None, | ||
directories: Directories::default(), | ||
} | ||
} | ||
|
@@ -118,7 +126,7 @@ impl FnmConfig { | |
} | ||
|
||
pub fn resolve_engines(&self) -> bool { | ||
self.resolve_engines | ||
self.resolve_engines.flatten().unwrap_or(true) | ||
} | ||
|
||
pub fn multishell_path(&self) -> Option<&std::path::Path> { | ||
|
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.
Related?