-
-
Notifications
You must be signed in to change notification settings - Fork 445
Remove UID and Field Instances #606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 2c94f77. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #606 +/- ##
==========================================
- Coverage 87.78% 87.59% -0.20%
==========================================
Files 31 31
Lines 819 822 +3
Branches 184 186 +2
==========================================
+ Hits 719 720 +1
- Misses 95 97 +2
Partials 5 5 ☔ View full report in Codecov by Sentry. |
Updating some of this code based on: #576 |
This reverts commit a0b5ea7.
<form.Subscribe | ||
selector={(state) => [state.canSubmit, state.isSubmitting]} | ||
children={([canSubmit, isSubmitting]) => ( | ||
<button type="submit" disabled={!canSubmit}> | ||
{isSubmitting ? '...' : 'Submit'} | ||
</button> | ||
)} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR
Currently, this part makes the CI fail, because of a bug in TypeScript 5.0.4
(4.9.1
has this error too, but we use a different tsconfig file for it, that excludes the useField.test.tsx
file from the compilation, so we don't see any errors for 4.9.1
in the CI).
🐛 The bug in TS 5.0.4
This is the type of form.Subscribe
(source):
Subscribe: <TSelected = NoInfer<FormState<TFormData>>>(props: {
selector?: (state: NoInfer<FormState<TFormData>>) => TSelected
children: ((state: NoInfer<TSelected>) => ReactNode) | ReactNode
}) => JSX.Element
In versions more recent than 5.0.4
, the type of theTSelected
generic would be inferred from the return type of the selector
method. If the returned value is [state.canSubmit, state.isSubmitting]
, then TSelected
will be boolean[]
. However, in 5.0.4
that kind of inference is broken, and TSelected
will be unknown
, falling back to the default value, which is NoInfer<FormState<TFormData>>
.
In the CI, the error we get for the highlighted code snippet complains about boolean[]
not being compatible with FormState<{ firstName: string; lastName: string; }
.
. test:types:versions50: src/tests/useField.test.tsx(585,17): error TS2322: Type '(state: FormState<{ firstName: string; lastName: string; }>) => boolean[]' is not assignable to type '(state: FormState<{ firstName: string; lastName: string; }>) => FormState<{ firstName: string; lastName: string; }>'.
. test:types:versions50: Type 'boolean[]' is missing the following properties from type 'FormState<{ firstName: string; lastName: string; }>': isFormValidating, isFormValid, errors, errorMap, and 11 more.
How should we solve this?
Fun fact, we can make form.Subscribe
work in TS 5.0.4
like this:
<form.Subscribe
{...{
selector: (state) => [state.canSubmit, state.isSubmitting],
children: ([canSubmit, isSubmitting]) => (
<button type="submit" disabled={canSubmit}>
{isSubmitting ? '...' : 'Submit'}
</button>
),
}}
/>
It looks ugly, unconventional and doesn't solve the underlying issue, so I wouldn't recommend it.
We could also use //ts-ignore
here and explain in a comment why we do it, but that might not prevent someone in the future from running into this very hard-to-debug problem when they contribute to this repo.
My favorite solution is following what we did for 4.9.5
: ignore the test files for 5.0.4
in the CI. I would also insert a comment at the type definition of form.Subscribe
to warn future collaborators (and our future selves) about this problem.
Obviously, we should mention this but in the docs too.
Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your outlined solution of ignoring the test files in 5.0.4, writing a comment on the type definition, and mentioning in the TypeScript docs page.
Any chance you want to tackle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd be happy to do it! I'll make a PR for it.
Edit: The PR is in progress, it has only one more task left ("mentioning in the TypeScript docs page") .
Edit2: 🎉 Now it's ready for review: #611
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one change suggestion that eliminates a non-null assertion, otherwise LGTM 🚀!
This PR probably fixed many unborn bugs by removing useIsomorphicEffectOnce.ts
. Great move before the v1
release! 🙌
This PR removes the concept of multiple
instances
of a Field for a givenname
, which in turn removes UID and, in happenstance, fixes #571.This should be good-to-go, but breaks the ability to have multiple
Field
items with the samename
. The question is if any of the @TanStack/form-maintainers can think of a reason why we might need that.