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

fix: .nvmrc version error #28655

Merged
merged 2 commits into from
Aug 21, 2023
Merged

fix: .nvmrc version error #28655

merged 2 commits into from
Aug 21, 2023

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar commented Aug 19, 2023

The current version string is producing following error in fnm tool:

Can't find an installed Node version matching >=18.16.0.
Do you want to install it? answer [y/n]: y
error: The requested version is not installable: >=18.16.0

Looks like nvm and fmn tools need a version string that will lead to exact version to install. So conditions like >= can't be used.

After fix it will look like this:

Can't find an installed Node version matching lts-hydrogen.
Do you want to install it? answer [y/n]: y
Installing Node v18.17.1 (x64)
Using Node for alias lts-hydrogen

Note: this will nag contributors install latest LTS of Hydrogen whenever they are available. Which may be good.

/cc @bsmth @Josh-Cena @sideshowbarker @caugner

@OnkarRuikar OnkarRuikar requested a review from a team as a code owner August 19, 2023 05:24
@github-actions github-actions bot added the system [PR only] Infrastructure and configuration for the project label Aug 19, 2023
@Josh-Cena
Copy link
Member

No opinions from me, although it is indeed slightly nonideal if it frequently needs new installs :) I wonder if we can update the nvm setup to make it support semver.

@OnkarRuikar
Copy link
Contributor Author

I wonder if we can update the nvm setup to make it support semver.

I don't think that is possible:

no, we don’t support that - everything in nvm resolves to one single version.

generally I’d expect everyone in an org to be on the exact same version - not just in a range. Why wouldn’t you want everyone on the latest 18?

@Josh-Cena
Copy link
Member

Josh-Cena commented Aug 20, 2023

The nvm setup is essentially a custom bash script:

cdnvm() {
    command cd "$@" || return $?
    nvm_path=$(nvm_find_up .nvmrc | tr -d '\n')

    # If there are no .nvmrc file, use the default nvm version
    if [[ ! $nvm_path = *[^[:space:]]* ]]; then

        declare default_version;
        default_version=$(nvm version default);

        # If there is no default version, set it to `node`
        # This will use the latest version on your machine
        if [[ $default_version == "N/A" ]]; then
            nvm alias default node;
            default_version=$(nvm version default);
        fi

        # If the current version is not the default version, set it to use the default version
        if [[ $(nvm current) != "$default_version" ]]; then
            nvm use default;
        fi

    elif [[ -s $nvm_path/.nvmrc && -r $nvm_path/.nvmrc ]]; then
        declare nvm_version
        nvm_version=$(<"$nvm_path"/.nvmrc)

        declare locally_resolved_nvm_version
        # `nvm ls` will check all locally-available versions
        # If there are multiple matching versions, take the latest one
        # Remove the `->` and `*` characters and spaces
        # `locally_resolved_nvm_version` will be `N/A` if no local versions are found
        locally_resolved_nvm_version=$(nvm ls --no-colors "$nvm_version" | tail -1 | tr -d '\->*' | tr -d '[:space:]')

        # If it is not already installed, install it
        # `nvm install` will implicitly use the newly-installed version
        if [[ "$locally_resolved_nvm_version" == "N/A" ]]; then
            nvm install "$nvm_version";
        elif [[ $(nvm current) != "$locally_resolved_nvm_version" ]]; then
            nvm use "$nvm_version";
        fi
    fi
}

Which means it's possible to write our own custom logic that replaces the $(nvm current) != "$locally_resolved_nvm_version" line. Of course it would mean some of us use a different config from everyone else, but that sounds acceptable.

.nvmrc Outdated Show resolved Hide resolved
@OnkarRuikar
Copy link
Contributor Author

Which means it's possible to write our own custom logic that replaces the $(nvm current) != "$locally_resolved_nvm_version" line. Of course it would mean some of us use a different config from everyone else, but that sounds acceptable.

One can also run echo '/.nvmrc' >> .git/info/exclude and use the version they want.
Or run touch .my_nvmrc && echo '/.my_nvmrc' >> .git/info/exclude and update the nvm script like this:

elif [[ $(nvm current) != "$locally_resolved_nvm_version" ]]; then
    nvm use $(cat .my_nvmrc);
fi

Down side is that they'll have to figure out themselves when repo needs newer version of NodeJs.

@sideshowbarker
Copy link
Collaborator

As an alternative, I’ve opened mdn/yari#9517.

Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Testing locally and works fine 👍🏻

@bsmth
Copy link
Member

bsmth commented Aug 21, 2023

nonideal if it frequently needs new installs

Hopefully this doesn't change often at all, although we did have issues in other minor versions: mdn/yari#9384

@OnkarRuikar
Copy link
Contributor Author

nonideal if it frequently needs new installs

Hopefully this doesn't change often at all, although we did have issues in other minor versions: mdn/yari#9384

Using 18.17 because it is the latest version which is >18.16 and it is what being used in yari deployment workflows so it works fine.

@bsmth
Copy link
Member

bsmth commented Aug 21, 2023

Thanks all, I'm going to merge this one now

@bsmth bsmth merged commit 320f0c1 into mdn:main Aug 21, 2023
15 checks passed
@OnkarRuikar OnkarRuikar deleted the fix_nvm branch August 21, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
system [PR only] Infrastructure and configuration for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants