Skip to content

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

Merged
Schniz merged 25 commits into
Schniz:masterfrom
amitdahan:support-engines
Jul 6, 2023
Merged

Schniz merged 25 commits into
Schniz:masterfrom
amitdahan:support-engines

Conversation

@amitdahan

@amitdahan amitdahan commented Oct 21, 2022 •

Copy link
Copy Markdown
Collaborator

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

changeset-bot Bot commented Oct 21, 2022 •

Copy link
Copy Markdown

🦋 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

ljharb commented Oct 21, 2022

Copy link
Copy Markdown

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.

@Schniz Schniz left a comment

Copy link
Copy Markdown
Owner

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?

Comment thread src/config.rs Outdated
Comment thread src/config.rs Outdated
@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

kud commented Sep 10, 2023

Copy link
Copy Markdown

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

Schniz commented Sep 11, 2023

Copy link
Copy Markdown
Owner

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

@Schniz

Schniz commented Sep 11, 2023

Copy link
Copy Markdown
Owner

If not, let us know

@amitdahan

Copy link
Copy Markdown
Collaborator Author

Yep, just add --resolve-engines:

(I'm using Zsh)

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

@kud

kud commented Sep 11, 2023

Copy link
Copy Markdown

Oh wow, indeed, simple!

Thank you guys!

@kud

kud commented Sep 11, 2023

Copy link
Copy Markdown

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

Schniz commented Sep 11, 2023

Copy link
Copy Markdown
Owner

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

@Schniz

Schniz commented Sep 11, 2023

Copy link
Copy Markdown
Owner

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
Copy Markdown

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

This branch was successfully deployed

1 active deployment
Preview — 88bc36ac Deployed Jul 6, 2023 by vercel[bot]
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