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

Test all supported versions of node #52

Closed
wants to merge 5 commits into from

Conversation

stephenmichaelf
Copy link
Member

#42

This change adds a way for us to test all supported node versions locally.

@bryanmacfarlane
Copy link
Contributor

Works fine on windows? I didn't think nvm worked on windows??

@stephenmichaelf
Copy link
Member Author

Yes sir, works on Windows. I use this one which I believe functions the same as the non-windows version. Did the tests work on Mac?

fail('requires nvm to be installed');
}

var versionsToTest = ['8.9.1', '6.12.0', '4.8.6'];
Copy link
Contributor

@bryanmacfarlane bryanmacfarlane Jan 5, 2018

Choose a reason for hiding this comment

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

This is already out of date. 6.12.3 is latest LTS.
Why not simplify all this logic here and simply use, 4, 6, 8
Then run nvm install x and nvm use x everytime.

The only time nvm install will take a time hit is when there's a new version and that's what we want. Otherwise, it will short circuit with already installed.

Copy link
Member Author

@stephenmichaelf stephenmichaelf Jan 5, 2018

Choose a reason for hiding this comment

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

According to this we should be able to say "nvm install 8" and get the latest LTS but it's not working(nvm install 8 tries to install 8.0.0), at least for the Windows version. I am guessing there are differences in the versions but not sure.

One alternative would be to do "nvm list available" and parse that, but it seems like overkill. We could also make a REST call I am guessing but that too seems like overkill. I will keep looking into this.

Copy link
Member Author

Choose a reason for hiding this comment

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

After looking here it looks like the Windows version behaves differently, I requested a change but looks like we will need a workaround that works cross plat.

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed offline, plan is to make it work for Linux/OSX and the command will do nothing on Windows.

@stephenmichaelf
Copy link
Member Author

Discussed offline, we are removing this feature.

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.

None yet

2 participants