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.

Sign up for GitHub

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

Jump to bottom

Introduce version alias for current node version and project pined version #1

Merged
merged 3 commits into from
Apr 7, 2020
Merged

Introduce version alias for current node version and project pined version #1

merged 3 commits into from
Apr 7, 2020

Conversation

AdrieanKhisbe
Copy link
Contributor

Support version alias for process version '-' and '.' for common project version locks (.node-version,.nvmrc,.naverc)

This is a Follow up from our discussions @ehmicky on client library get-node: ehmicky/get-node#4

So here is the Proposal:

  • Problem addressed: with nve, running on the .nvmrc version of project and others, without having to explicitly say which exact my .nvmrc is

  • Related Prs: no pr opened

  • Solution proposed: support of aliases/pseudo versions:

    • .: resolve to the version range stored in .node-version, .nvmrc or .naverc (using find-up)
    • _ : resolve to current process node version
    • maybe more, I'm tempted to hook into nvm aliases if available (for lts or user defined)

  • Checklist

    • I have read the contribution guidelines.
    • I have added tests (we are enforcing 100% test coverage).
    • I have added documentation in the README.md, the docs directory (if
      any) and the examples directory (if any).
    • The status checks are successful (continuous integration).

@codecov
Copy link

codecov bot commented Apr 1, 2020 •
edited
Loading

Codecov Report

Merging #1 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
src/aliases.js 100.00% <100.00%> (ø)
src/main.js 100.00% <100.00%> (ø)
Flag Coverage Δ
#linux 100.00% <100.00%> (ø)
#mac 100.00% <100.00%> (ø)
#node_v10_17_0 100.00% <100.00%> (ø)
#node_v13_12_0 100.00% <100.00%> (ø)
#windows 100.00% <100.00%> (ø)

ehmicky
ehmicky reviewed Apr 3, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 3, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 3, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 3, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 3, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 3, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 3, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 3, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 3, 2020
View reviewed changes
src/aliases.js Outdated

if (!nodeVersionFile) {
throw new Error(
`node config file not found [was looking for ${NODE_VERSION_FILES.join(
Copy link
Owner

@ehmicky ehmicky Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

When no NODE_VERSION_FILES was found, we should default to process.version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did. it, but I'm a bit unsure about it.

That is a good default, but it should be disableable. Otherwise no way to know if version was resolved from version file, or if just process version.

Copy link
Owner

Choose a reason for hiding this comment

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

I think this is an interesting idea, but I would favor simplicity (no options) over feature here as a first step. However if someone post an issue where they wished that an error was thrown to notify them that they forgot to add their .nvmrc, we should add that option then.

ehmicky
ehmicky requested changes Apr 3, 2020
View reviewed changes
Copy link
Owner

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR @AdrieanKhisbe!

I added some comments.
Also, the CI is currently failing. I think this is because the new function is not used by src/main.js yet.

Thanks for your help!

ehmicky
ehmicky reviewed Apr 3, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 3, 2020
View reviewed changes
@AdrieanKhisbe AdrieanKhisbe force-pushed the support-nvmrc-pseudo-version branch from dd1fc27 to 6627c6a Compare April 3, 2020 19:43
@AdrieanKhisbe
Copy link
Contributor Author

I made the changes, but then just realize I had an outdated version of the page, missing lasts changes. 🤦‍♂

I'll be back

AdrieanKhisbe added a commit to AdrieanKhisbe/normalize-node-version that referenced this pull request Apr 3, 2020
@AdrieanKhisbe
Applied suggestions on PR ehmicky#1 and made '_' the default for '.' …
5e1b670 
…when no version file
@AdrieanKhisbe
Copy link
Contributor Author

@ehmicky I completed the changes, and introduced an alias section in the documentation:

I set . default to process.version (but we some prevent default would be useful, and reintroduce Error from initial verison in that case) cf

I might try to work on nvm aliases support. Tell me if you prefer that to happens in that PR, or a second one, once thos one has landed. 🙂

@AdrieanKhisbe
Copy link
Contributor Author

@ehmicky I did work on nvm alias in the end, you can preview it here:
AdrieanKhisbe/normalize-node-version@support-nvmrc-pseudo-version...AdrieanKhisbe:support-nvm-aliases

(tests are green except on windows where is an issue cloning the repo cause by one nvm fixture file)

ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
ehmicky
ehmicky reviewed Apr 4, 2020
View reviewed changes
@AdrieanKhisbe
Copy link
Contributor Author

@ehmicky I have all changes ready but the one about the two test were I used `resolveVersionRangeAlias purposefully: #1 (comment)

I before to commit and push, I would like to know if it's okey, or if you really prefer not to unit test at this level.

I'll wait for your input on this issue to commit and push (I'll also apply the rebase that needs to be done with the current package-lock.json conflict)

@AdrieanKhisbe
Support version aliases for process version '-' and '.' for common pr…
67ea6e2 
…oject version locks

Supported files for node locks: .node-version, .nvmrc, .naverc

Follow up discussion on client library get-node: ehmicky/get-node#4
@AdrieanKhisbe
Applied suggestions on PR ehmicky#1 and made '_' the default for '.' …
c23e08a 
…when no version file
@AdrieanKhisbe
Applied second&third round of @ehmicky suggestion changes on ehmicky#1
ac12c5b
@AdrieanKhisbe AdrieanKhisbe force-pushed the support-nvmrc-pseudo-version branch from 8d78643 to ac12c5b Compare April 7, 2020 07:39
@AdrieanKhisbe
Copy link
Contributor Author

@ehmicky,and here it is :)

@AdrieanKhisbe
Copy link
Contributor Author

(strange the lint is breaking, it's passing on my machine, and my CI 🤔 https://github.com/AdrieanKhisbe/normalize-node-version/actions/runs/72487439)

@ehmicky ehmicky self-requested a review April 7, 2020 11:43
ehmicky
ehmicky approved these changes Apr 7, 2020
View reviewed changes
Copy link
Owner

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR @AdrieanKhisbe! 🎉
This looks good now. There is a linting issue and a merge conflict with package-lock.json but I will fix it directly on master.

@ehmicky ehmicky merged commit 50be4e8 into ehmicky:master Apr 7, 2020
@ehmicky
Copy link
Owner

ehmicky commented Apr 7, 2020 •
edited
Loading

Released in:

🎉

@all-contributors Could you please add @AdrieanKhisbe for code, test and ideas?

@allcontributors allcontributors bot mentioned this pull request Apr 7, 2020
Merged
@allcontributors
Copy link
Contributor

@ehmicky

I've put up a pull request to add @AdrieanKhisbe! 🎉

@AdrieanKhisbe AdrieanKhisbe deleted the support-nvmrc-pseudo-version branch April 7, 2020 20:48
@AdrieanKhisbe
Copy link
Contributor Author

@ehmicky Awesome! :)
Thanks a lot too! =)

And interesting this @allcontributor bot!

AdrieanKhisbe added a commit to AdrieanKhisbe/normalize-node-version that referenced this pull request Apr 13, 2020
@AdrieanKhisbe
Support for nvm aliases (including lts/aliases, stable, personal ones…
48147c5 
…, indirections and cycle protection) 🔖

🛠️ This is a rework of https://github.com/AdrieanKhisbe/normalize-node-version/commits/support-nvm-aliases--original prototype when ehmicky#1 was still in building 🏗️
AdrieanKhisbe added a commit to AdrieanKhisbe/normalize-node-version that referenced this pull request Apr 13, 2020
@AdrieanKhisbe
Support for nvm aliases 🔖
de550f2 
includies lts/aliases, stable, personal ones, indirections and cycle protection

:hammer_and_wrench: This is a rework of https://github.com/AdrieanKhisbe/normalize-node-version/commits/support-nvm-aliases--original prototype when ehmicky#1 was still in building 🏗️
This was referenced Apr 13, 2020
Closed
Closed
AdrieanKhisbe added a commit to AdrieanKhisbe/normalize-node-version that referenced this pull request Apr 13, 2020
@AdrieanKhisbe
Support for nvm aliases 🔖
cdc0b73 
includies lts/aliases, stable, personal ones, indirections and cycle protection

:hammer_and_wrench: This is a rework of https://github.com/AdrieanKhisbe/normalize-node-version/commits/support-nvm-aliases--original prototype when ehmicky#1 was still in building 🏗️
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehmicky ehmicky ehmicky approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AdrieanKhisbe @ehmicky