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

Add 'node' alias for nvm compat #5

Merged
merged 1 commit into from
Aug 28, 2020
Merged

Conversation

dcodeIO
Copy link
Contributor

@dcodeIO dcodeIO commented Apr 9, 2020

This adds the alias node for current for compatibility with nvm, where node "installs the latest version of node".

My use case here is that I'd like to put this library in front of nvm respectively nvm-windows, which don't behave exactly the same, so this would help to achieve better compatibility.

@wesleytodd
Copy link
Member

I actually have an open issue with nvm for this to be the other way around. The Package Maintenance WG is working on this as an official definition, and I would really hope @ljharb would get behind using our aliases.

I am happy to make the ergonomic changes to this library, but the whole point it to standardize on this set as defined by the working group.

@ljharb
Copy link
Member

ljharb commented Apr 9, 2020

I don't think it makes sense to ever use aliases that follow support timelines when attempting to locate a specific version of node - only when attempting to determine support for one.

@dcodeIO
Copy link
Contributor Author

dcodeIO commented Apr 10, 2020

This PR is especially easy to work around, so not particularly urgent. I do agree that current is more ergonomic than node, and having that in nvm would be great as well. Perhaps, if that happens it might still have a benefit to add the alias here for compatibility with earlier versions of nvm?

@wesleytodd
Copy link
Member

I don't think it makes sense to ever use aliases that follow support timelines when attempting to locate a specific version of node - only when attempting to determine support for one.

@ljharb I am not sure I understand your meaning? If I do nvm install lts_active it is just a point in time resolve and install. This is the exact same as the current feature for lts/*, the difference only the standardizing around these other strings.

if that happens it might still have a benefit to add the alias here

I agree that it probably valuable. I will let it sit for a few more days to give folks like @dominykas a chance to chime in (as the original author of the recommendation). But then will merge it up!

@dominykas
Copy link
Member

I think this should reflect what people use in real life - and I've seen node used as a keyword in .travis.yml configs, and nvm use node does work, so I think it fits to have that alias here.

@shadowspawn
Copy link

shadowspawn commented Apr 16, 2020

I have a reservation about this PR. Is this package implementing the prospective official aliases, or will it continue adding compatibility for nvm historical alias usage for ergonomic reasons? Will you entertain aliases from other node version managers or CI conventions?

I second the comments in #5 (comment) and in particular this comment:

the whole point it to standardize on this set as defined by the working group.

(On a related note, I do also question the support for `lts/* mentioned in the README here for the same reason!)

@dcodeIO
Copy link
Contributor Author

dcodeIO commented Apr 16, 2020

That's a good point, and makes a lot of sense to me as well.

@wesleytodd
Copy link
Member

I think this should reflect what people use in real life - and I've seen node used as a keyword in .travis.yml configs, and nvm use node does work, so I think it fits to have that alias here.

So node is a weird alias, so while I understand for backward compatibility we should add it in 1.0.0, but long run I think we need to remove support for this alias in all the tools in favor of the more explicit ones as defined in the PM WG doc.

I have not had time to follow up on the outcome from nvm-sh/nvm#2170, but if that is done I hope we can take this approach even in nvm. The only person I have seen opposed to this so far is @ljharb, so I would like to hear again from him on this.

@shadowspawn
Copy link

I see the tag line for this repo is currently: "Tool for getting node versions by common aliases" which is a wider brief than I had assumed. (Thanks for comments.)

I withdraw my reservation. 😄

@ljharb
Copy link
Member

ljharb commented Apr 17, 2020

@wesleytodd i think aliases that vary based on the content of "the list of available versions" is fine. I think aliases that have to do any datetime math whatsoever are not fine.

If the node build team is willing to add a bit to the index.tab that indicates EOL status of a version, then that would be fine for nvm to pivot on.

@wesleytodd
Copy link
Member

Awesome, so as I said before then, that is the next step. If anyone else has time to move that forward, awesome! I will merge this so we get backwards compat and we can plan to remove it in a future major when the other tools follow the aliases!

@dominykas
Copy link
Member

Should we merge this?

@wesleytodd
Copy link
Member

I will put together a release now, sorry for the delay!

@wesleytodd wesleytodd merged commit ed601e6 into pkgjs:master Aug 28, 2020
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