[WIP] Alias support #8
Conversation
|
Changed to use |
|
Build failures appear to be related to wbyoung/avn#66. All tests and jshint checks pass locally. |
| @@ -9,6 +9,32 @@ var name = require('./package.json').name; | |||
|
|
|||
| var VERSION_REGEX = /(\w+)-(.+)/; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
This comment appears inaccurate; if not present, it seems to be a rejected promise.
| * | ||
| * @param {Promise} matching | ||
| */ | ||
| var parseMatching = function(matching) { |
There was a problem hiding this comment.
String(x) is always preferable to x.toString()
|
Great, thanks for the feedback @ljharb. I've fixed those items. |
|
Looks great to me! Can you rebase on master to get the tests passing? |
This requires altering the stubbed child.spawn method to return different responses based on the arguments.
1 similar comment
1 similar comment
|
@wbyoung Added a test for the case when the system node is not found, so the coverage is back up to 100% now. |
| }) | ||
| .then(function(useVersion) { | ||
| var use = (useVersion && useVersion.indexOf('system') === 0) ? | ||
| 'system' : useVersion; |
There was a problem hiding this comment.
This has the ability to conflict with an alias defined as something like system-x, doesn’t it?
Perhaps the then handler above that decides to get the system node should also update a variable define in the scope of the match function body to store that choice & use it here.
There was a problem hiding this comment.
@wbyoung Actually, in this case useVersion will only ever start with system if the .nvmrc string is simply system, and not if it is something like system-x. The reason is that resolveVersion will resolve to an actual version number for aliases like system-x, which will then be propagated throughout the rest of the match method. I've added a test to check that this is the case in 42a4dbb.
|
Anything left here to get this merged? |
|
Is there a release slated for this? |
|
@mattduggan just published |
|
@wbyoung Thank you for publishing this! Congrats & enjoy the time! |
|
Thank you @wbyoung! And congrats! |
Adds support for aliased version names recognized by
nvmincludinglts/<release_name>,default,system.Changes to Tests
Since we are now making more calls using
child.spawnthan just thenvm listcommand it was necessary to be more specific in howsinonmocked each all. ThebeforeEachhook now has logic to return different mocked responses based on the arguments.Also added tests for alias and system support
Resolving Aliases v. System Node
The
systemalias case had to be handled separately from other aliases, primarily due to the difference in the waynvmhandles those two cases. For most aliases, we usenvm version "<alias_name>"to resolve a specific installed version number, thennvm use <version_number>to activate that version. However, withsystemwe need to usenvm use systemrather than the specific version the system node, since that version was not installed vianvmand therefore will throw an error.I wanted to be able to still log the actual version of node being used to keep things consistent with the logging for other version types, so I ended up propagating the value of
systemdown through all of the methods that resolve a version and only find the actual version number at the end so that we can log out that info.Addresses wbyoung/avn#43, wbyoung/avn#53
@wbyoung Let me know what suggestions/feedback you have.