Added command option to disable type-casting #268
Conversation
Conflicts: dist/command.js dist/option.js lib/command.js
Rename variable from Vantage to Vorpal
Updated dev deps and switched to Yarn
Updated Inquirer to v3 and other prod deps
…values Support default values for options
Remove dist folder
…ig for Babel and ESLint.
Update babel/eslint build config
Convert commands to ES2015
Update lib/history.js to ES2015
Converted UI to ES2015
Under some rare conditions, Vorpal can produce an invalid persisted history which, when loaded, would result in an uncaught exception that crashes the caller. To prevent this, the history is reset when invalid JSON is loaded.
Fixed crash when persisted history cannot be parsed
Convert utils to ES2015
lib/utils/buildCommandArgs.js
Outdated
| @@ -56,7 +56,7 @@ export default function buildCommandArgs( | |||
| }); | |||
|
|
|||
| // Use minimist to parse the args, and then build varidiac args and options. | |||
| const parsedArgs = parseArgs(passedArgs, types); | |||
| const parsedArgs = parseArgs(passedArgs, types, command._disableTypeCasting === true); | |||
There was a problem hiding this comment.
Wouldn't it make more sense to remove the type casting option from this types object? (types are options passed to minimist: https://github.com/substack/minimist#var-argv--parseargsargs-opts).
There was a problem hiding this comment.
We could indeed provide access to the types array, but I think that's too low level, and it requires users to manually add each flag to this list (and to do it again whenever a flag change). This disableTypeCasting() method is higher level and easier to use. It's even possible to disable type casting and then to manually do type casting in the action method.
Also for users who don't want type casting at all anywhere, it makes it easier to disable it by looping on the commands and calling this method.
This is a bit similar to what Vorpal is already doing for flags without arguments, where it adds them automatically to types.booleans for convenience.
|
Is there any chance this pull request could be merged in one form or another? I keep running into issues due to Minimist automatic casting. For example, most of the time the arguments are going to be strings so it's fine to use functions like "indexOf" on them, but in rare cases the argument is going to be converted to a number, which makes string functions suddenly fail. So I need to cast to strings every time when getting an argument from Vorpal. You basically can't expect any consistency and It's quite a annoying bug that can remain undetected for a long time in a program. Is there maybe some changes I could make to the pull request to get it accepted? |
|
@laurent22 The master branch is currently in development for v2, so merging this in and tagging a new release isn't possible at this time. We're also planning to remove minimist all together. |
|
I moved 2.0 code to a branch and reset master. Sorry, but you'll need to rebase or start this PR again. |
|
Ok that's good news actually if you are going to remove minimist. Is there any roadmap for v2.0? (just to get some idea of what's coming and maybe contribute) |
|
any workaround? I would like to preserve my input as string, not number |
|
For those looking for solution. This will make all arguments of the command to be parsed as strings: |
By default, the underlying command parser package (minimist), converts strings that look like numbers to actual numbers. This causes all kind of problems since it's also going to randomly convert things like hashes to numbers.
For example, a hash like "a1ef" would result in "a1ef", but another hash like "93e8" would result in "9300000000". There are quite a few pull requests about this in the minimist package GitHub page, but unfortunately it's no longer maintained so it looks like it won't be fixed.
This pull request attempts to fix this in a way that preserves backward compatibility. It adds a command option
disableTypeCasting()which, when used, disables type casting for all arguments and all flags for that command. If that option is not set, the code path is exactly the same so the applications that don't explicitly opt-in won't be affected.