fix(react-form): default selector to identity in Subscribe component#2065
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.
There was a problem hiding this comment.
Could you explain why using a subscription without a selector makes no sense? In some cases, I only want the content inside a Subscribe component (usually a submit button) to react to multiple form states like isValid, isSubmitting, isTouched.....
Yes, I could return all of these from a selector, but then I’d have to keep updating the selector whenever I want to listen to additional states. Personally, I don’t mind if that button re-renders more often than strictly necessary.
| 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.
|
I checked through some source code again, and it looks like it is explicitly marked as optional in multiple places. This should still be changed at some point, but since it looks intentional and has runtime implications, we'll keep it as optional. Thanks for the adjustment! |
|
Good question. The reasoning is about preventing unintentional performance issues by default. Without a selector, Your use case is valid: a submit button watching <Subscribe selector={(state) => state}>
{/* I know this re-renders often */}
</Subscribe>vs. accidentally forgetting a selector and wondering why performance degrades. That said, if the maintainers prefer allowing |
|
I think the decision is clear that it should always be granular and specified. However, given the semantic versioning, that's not going to be possible in v1. It will be noted for v2. |
3 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