fix: Replace raw in operator checks with keyof-validated type guards#4306
fix: Replace raw in operator checks with keyof-validated type guards#4306
in operator checks with keyof-validated type guards#4306Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4306 +/- ##
=======================================
Coverage 97.43% 97.43%
=======================================
Files 896 897 +1
Lines 26298 26352 +54
Branches 9496 9509 +13
=======================================
+ Hits 25623 25677 +54
Misses 632 632
Partials 43 43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
georgylobko
left a comment
There was a problem hiding this comment.
In overall I'm not following what is fundamentally different in this function after refactoring. Could you add a u tests that fails before the refactoring and passes after?
| type SearchableTagFields = 'tags' | 'filteringTags'; | ||
|
|
||
| const isGroup = (option: AutosuggestItem) => 'type' in option && option.type === 'parent'; | ||
| const isGroup = (option: AutosuggestItem) => option.type === 'parent'; |
There was a problem hiding this comment.
the type is optional
| const isGroup = (option: AutosuggestItem) => option.type === 'parent'; | |
| const isGroup = (option: AutosuggestItem) => option?.type === 'parent'; |
| const useEntered = 'type' in option && option.type === 'use-entered'; | ||
| const isParent = 'type' in option && option.type === 'parent'; | ||
| const isChild = 'type' in option && option.type === 'child'; | ||
| const useEntered = option.type === 'use-entered'; |
There was a problem hiding this comment.
| const useEntered = option.type === 'use-entered'; | |
| const useEntered = option?.type === 'use-entered'; |
There was a problem hiding this comment.
And the same for isParent and isChild
| return key in item; | ||
| } | ||
|
|
||
| export function isRefObject<T>(ref: React.Ref<T>): ref is React.RefObject<T> { |
There was a problem hiding this comment.
Should the ref argument be typed as any? When you call this function, you don't know the type of ref yet - that's exactly what this function is supposed to check
| export const isGroup = (option?: OptionDefinition | OptionGroup): option is OptionGroup => | ||
| !!option && 'options' in option && !!option.options; | ||
| export const isGroup = (option?: OptionDefinition | OptionGroup): option is OptionGroup => { | ||
| const key: keyof OptionGroup = 'options'; |
There was a problem hiding this comment.
I'm not following what is fundamentally different in this function after refactoring. Could you add a u test that fails before the refactoring and passes after?
Description
Replaces raw
'prop' in objchecks withkeyof-validated alternatives across 6 components to prevent a class of bugs where TypeScript silently accepts invalid property name strings ininexpressions.This is a COE action item AWSUI-59006 from the property filter bug (YpPBAPz2BrqF) where a type change during refactoring silently broke a
'propertyKey' in newTokencheck because TypeScript doesn't validate theinoperator —'banana' in objcompiles without error.Changes:
isStackableItem()andisRefObject()type guards toflashbar/utils.tscollapsible-flashbar.tsxto useisStackableItem()(2 checks) andisRefObject()(1 check)non-collapsible-flashbar.tsxto useisRefObject()(1 check)mixed-line-bar-chart/utils.tsto usekeyofinisYThreshold()andisXThreshold()(2 checks)side-navigation/util.tsxto use inlinekeyofvariables for'href'and'items'checks (2 checks)top-navigation/parts/utility.tsxto use inlinekeyoffor'items'check (1 check)button-dropdown/internal.tsxto use inlinekeyoffor'items'and'badge'checks (2 checks)button-group/item-element.tsxto use inlinekeyoffor'popoverFeedback'check (1 check)12 checks across 7 files, 6 components. No behavioral changes. Pure refactor for type safety.
Related links, issue #, if available:
AWSUI-59006and Tech Proposal. (k9wvA3eNMiZ9).How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.