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 897 897
Lines 26350 26358 +8
Branches 9516 9516
=======================================
+ Hits 25675 25683 +8
+ Misses 669 632 -37
- Partials 6 43 +37 ☔ 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?
src/autosuggest/utils/utils.ts
Outdated
| 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
src/flashbar/utils.ts
Outdated
| 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?
a4a78de to
41ef22e
Compare
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.