fix: make cancel action actually cancel even when there are validation errors #541
Conversation
🦋 Changeset detectedLatest commit: 6cfaaea The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -11,7 +12,6 @@ import { | |||
| previewJSONAction, | |||
| previewYAMLAction, | |||
| } from './defs' | |||
| import { red } from 'chalk' | |||
There was a problem hiding this comment.
Using our colors proxy module makes unit testing easier.
| @@ -76,7 +77,7 @@ export const updateFromUserInput = async <T extends object>(command: SmartThings | |||
| name: 'action', | |||
| message: 'Choose an action.', | |||
| choices, | |||
| default: validationResult === true ? finishAction : editAction, | |||
There was a problem hiding this comment.
When working on unit tests (still in progress), I noticed there is no way to get to this point if validationResult is not true.
| const propertyInputDefinition = inputDefsByProperty[propertyName] | ||
| if (!propertyInputDefinition) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
I noticed this code wasn't doing anything while I was working on unit tests.
| const propertyInputDefinition = inputDefsByProperty[propertyName] | ||
| if (!propertyInputDefinition) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
I noticed this code wasn't doing anything while I was working on unit tests.
| @@ -218,7 +214,7 @@ export function objectDef<T extends object>(name: string, inputDefsByProperty: I | |||
| const laterPropertyInputDefinition = inputDefsByProperty[propertyName] | |||
| const updateIfNeeded = laterPropertyInputDefinition.updateIfNeeded | |||
| if (updateIfNeeded) { | |||
| const laterPropertyValue = await updateIfNeeded(updated[propertyName], updatedPropertyName, [{ ...updated }, ...context]) | |||
| const laterPropertyValue = await updateIfNeeded(updated[propertyName], action, [{ ...updated }, ...context]) | |||
There was a problem hiding this comment.
Another thing I noticed while working on the unit tests. The updatedPropertyName is fine for top-level properties (line 105) but for nested properties, it only tells us which object the nested property is in and not property name inside that object that is actually being edited.
A real-world example of this is the clientName definition on line 58 of the apps:create command (packages/cli/src/commands/apps/create.ts). Before this fix, it would pass just oauth but after this it will pass oauth.clientName.
(There is currently no code that uses this so the difference doesn't matter ATM but we should be passing on everything.)
| input3: 'Item Value 3', | ||
| }]) | ||
| }) | ||
| it('calls `updateIfNeeded` on subsequent definitions after rolled-up property has been updated', async () => { |
There was a problem hiding this comment.
Should have a line break before the beginning of this test
Checklist
npm run lintproduces no warnings/errors)