fix(react-form): default selector to identity in Subscribe component#2065
fix(react-form): default selector to identity in Subscribe component#2065isaackaara wants to merge 3 commits intoTanStack:mainfrom
Conversation
When <form.Subscribe> is rendered without a selector prop, it crashes at runtime because useStore receives undefined as the selector: TypeError: selector is not a function The types already mark selector as optional, so the runtime behavior should match. This adds a default identity function ((state) => state) for the selector parameter in both LocalSubscribe implementations (useForm and useFieldGroup). Fixes TanStack#2063
|
|
View your CI Pipeline Execution ↗ for commit e2e3aa7
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2065 +/- ##
===========================================
- Coverage 90.35% 56.50% -33.85%
===========================================
Files 38 18 -20
Lines 1752 246 -1506
Branches 444 47 -397
===========================================
- Hits 1583 139 -1444
+ Misses 149 92 -57
+ Partials 20 15 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
dw about the coverage failing, as long as the lines you changed are covered. I'll review in a bit. Thanks! |
LeCarbonator
left a comment
LeCarbonator
left a comment
There was a problem hiding this comment.
Looks good! As far as runtime changes go, this is ready to merge. I'm not convinced by the type change, and since type changes are not covered by semantic versioning, those are fine to keep.
Curious to hear your thoughts.
| children, | ||
| }: PropsWithChildren<{ | ||
| lens: AnyFieldGroupApi | ||
| selector: (state: FieldGroupState<any>) => FieldGroupState<any> |
There was a problem hiding this comment.
You mentioned that "the types already mark the selector as optional", but it wasn't marked here.
A subscription without a selector (or an identity selector) makes no sense, so we shouldn't encourage it. For the sake of semver, the identity function fallback should stay, but the types should enforce that a selector is required.
| expect(button).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('should render FieldGroup Subscribe without selector (default identity)', async () => { |
There was a problem hiding this comment.
Of course, if the selector will be marked as required, these tests would type error.
Adjust the title to mention the related GH issue or that this is for semver.
2 participants
When
<form.Subscribe>is rendered without aselectorprop, it crashes at runtime:The types already mark
selectoras optional (selector?), so the runtime should handle the missing case. This adds a default identity function(state) => statefor theselectorparameter in bothLocalSubscribeimplementations (useFormanduseFieldGroup).Before (crashes):
After (works, returns full form state):
Fixes #2063