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

Support engines field when .nvmrc / .node-version are not present #839

Merged
merged 25 commits into from
Jul 6, 2023

Conversation

amitdahan
Copy link
Collaborator

@amitdahan amitdahan commented Oct 21, 2022

Summary

Adds support for engines field in package.json so having the following is supported with fnm use and fnm install:

{
  "name": "my-thing",
  "engines": {
    "node": ">= 13.3.7 < 14.0.0"
  }
}

(Relies on #816 and #991)

Since this isn't directly equivalent to the "locked" nature of .nvmrc or .node-version, this feature will start as experimental, and behind an opt-in --resolve-engines:
image

@changeset-bot
Copy link

changeset-bot bot commented Oct 21, 2022

🦋 Changeset detected

Latest commit: 88bc36a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
fnm Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ljharb
Copy link

ljharb commented Oct 21, 2022

The engines field is a range, not a single version, and affects consumers - it’s not meant to be used for the developer’s node version.

If the feature only worked when the app is private: true, then at least that would avoid the risk of breaking consumers, but it’s still a conceptual mismatch to use a ranged field for a single version.

Copy link
Owner

@Schniz Schniz left a comment

Choose a reason for hiding this comment

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

lgtm overall. What do you think of my comments?

src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
@Schniz Schniz merged commit 97be792 into Schniz:master Jul 6, 2023
@github-actions github-actions bot mentioned this pull request Jul 6, 2023
@amitdahan amitdahan deleted the support-engines branch July 6, 2023 07:05
@kud
Copy link

kud commented Sep 10, 2023

That's really great. Is there any way to auto-detect this? My shell already starts fnm use when a config file is found.

@Schniz
Copy link
Owner

Schniz commented Sep 11, 2023

fnm env --use-on-cd works with this feature for auto detection

@Schniz
Copy link
Owner

Schniz commented Sep 11, 2023

If not, let us know

@amitdahan
Copy link
Collaborator Author

Yep, just add --resolve-engines:

(I'm using Zsh)

eval "$(fnm env --use-on-cd --resolve-engines)"

@kud
Copy link

kud commented Sep 11, 2023

Oh wow, indeed, simple!

Thank you guys!

@kud
Copy link

kud commented Sep 11, 2023

None works actually.

screenshot-2023-09-11-10-35-39@2x
eval "$(fnm env --use-on-cd --resolve-engines)" > /dev/null 2>&1

in zsh.

@Schniz
Copy link
Owner

Schniz commented Sep 11, 2023

yeah seems like it's broken on use-on-cd, let's open an issue and fix it

@Schniz
Copy link
Owner

Schniz commented Sep 11, 2023

it failed when I tested on a docker container, yet worked on my machine: it's because I'm using --version-file-strategy=recursive. I wonder if this should just become the default now. cc @amitdahan

@gotounix
Copy link

I found it not work on Windows (PowerShell):

$env:FNM_MULTISHELL_PATH = "C:\Users\Max\AppData\Local\fnm_multishells\7196_1708612586200"
$env:FNM_VERSION_FILE_STRATEGY = "local"
$env:FNM_COREPACK_ENABLED = "false"
$env:FNM_RESOLVE_ENGINES = "true"
$env:FNM_DIR = "C:\Users\Max\AppData\Roaming\fnm"
$env:FNM_NODE_DIST_MIRROR = "https://nodejs.org/dist"
$env:FNM_ARCH = "x64"
$env:FNM_LOGLEVEL = "info"
E:\test [master ≡ +1 ~1 -0 !]> fnm use --resolve-engines
error: Can't find version in dotfiles. Please provide a version manually to the command.

@Schniz

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.

5 participants