-
Notifications
You must be signed in to change notification settings - Fork 0
Fix useField's onChange function should accept non-string values as args #1
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
base: main
Are you sure you want to change the base?
Conversation
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.
Caution
Changes requested ❌
Reviewed everything up to c111a42 in 3 minutes and 0 seconds. Click for details.
- Reviewed
73
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
6
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/formik/src/Formik.tsx:536
- Draft comment:
An extra closing brace was added in the registerField callback. Ensure this change correctly terminates the callback without altering its intended scope. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. packages/formik/src/Formik.tsx:601
- Draft comment:
The executeChange signature now includes '| any' to support non-string values. Consider using a more specific union type if possible to avoid masking type issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment suggests improving type safety by using a more specific union type instead of "any". However, looking at the code, the "any" type seems intentional to handle multiple value types that can come from form inputs. The code explicitly checks for strings, arrays, numbers, booleans and objects. Creating a more specific union type might make the code more brittle without adding much value. I could be wrong about the value of type safety here - maybe there are specific form value types that should be enumerated explicitly. The "any" type does lose type checking benefits. While type safety is good, in this case the function needs to handle arbitrary form input values. The code already has explicit runtime type checks for the values it cares about. A more specific type would likely just mirror those runtime checks without adding practical value. The comment should be deleted. The use of "any" here appears intentional and appropriate given the function's need to handle multiple form value types, with runtime type checking already in place.
3. packages/formik/src/types.tsx:323
- Draft comment:
The onChange overload for FieldInputProps now supports non-string values. Please verify that the overload ordering does not introduce ambiguity, especially when Value might be a string. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify something, which violates the rule against asking for confirmation or verification. It doesn't provide a specific suggestion or point out a clear issue.
4. packages/formik/src/Formik.tsx:1216
- Draft comment:
The file lacks a newline at the end. Consider adding a newline for consistency with standard practices. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The rules state to only keep comments that require clear code changes and to not make purely informative comments. While missing newlines at end of file is a common style convention, it's a very minor issue that would likely be caught by linting or formatting tools. The comment doesn't point out a functional issue that needs fixing. The missing newline could cause issues with some tools and is considered a best practice by many style guides. Not having it could be seen as a real issue worth fixing. While it is a real style convention, this type of minor formatting issue should be handled by automated tools rather than manual review comments. The rules specifically say not to make purely informative comments. This comment should be removed as it points out a minor style issue that would be better handled by automated tools rather than manual review comments.
5. packages/formik/src/Formik.tsx:75
- Draft comment:
Typo: There is a missing newline between the 'SET_ISSUBMITTING' return statement and the next case ('SET_ISVALIDATING'). This should be split over two lines to improve readability and avoid potential issues. - Reason this comment was not posted:
Marked as duplicate.
6. packages/formik/src/Formik.tsx:1120
- Draft comment:
Minor Typographical Issue: There is no newline at the end of the file. It's a minor style issue that can be fixed automatically. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_ku1v7uu0N0c6pUzG
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@@ -72,8 +72,7 @@ function formikReducer<Values>( | |||
case 'SET_STATUS': | |||
return { ...state, status: msg.payload }; | |||
case 'SET_ISSUBMITTING': | |||
return { ...state, isSubmitting: msg.payload }; | |||
case 'SET_ISVALIDATING': | |||
return { ...state, isSubmitting: msg.payload };case 'SET_ISVALIDATING': |
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.
The switch-case formatting is confusing. The SET_ISVALIDATING
case is appended to the previous return. Consider placing it on its own line for clarity.
@@ -610,7 +610,7 @@ export function useFormik<Values extends FormikValues = FormikValues>({ | |||
let parsed; | |||
// If the first argument is not a string though, it has to be a synthetic React Event (or a fake one), | |||
// so we handle like we would a normal HTML change event. | |||
if (!isString(eventOrTextValue)) { | |||
if (!isString(eventOrTextValue) && !Array.isArray(eventOrTextValue) && typeof eventOrTextValue !== 'number' && typeof eventOrTextValue !== 'boolean' && typeof eventOrTextValue !== 'object') { |
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.
The new if condition in executeChange
may not correctly detect synthetic events. Since React events are objects, this check (which excludes objects) will bypass event handling. Consider checking for event-specific properties (e.g. 'target' or 'persist') instead.
if (!isString(eventOrTextValue) && !Array.isArray(eventOrTextValue) && typeof eventOrTextValue !== 'number' && typeof eventOrTextValue !== 'boolean' && typeof eventOrTextValue !== 'object') { | |
if (eventOrTextValue && (typeof eventOrTextValue === 'object') && (eventOrTextValue.target || eventOrTextValue.persist)) { |
Important
Enhances
useField
'sonChange
to accept non-string values and fixes a formatting issue informikReducer
.executeChange
inFormik.tsx
now acceptsstring
,React.ChangeEvent
, or any type, allowing non-string values.onChange
inFieldInputProps
intypes.tsx
updated to acceptReact.ChangeEvent
, field value type, orstring
.formikReducer
inFormik.tsx
by separatingSET_ISSUBMITTING
andSET_ISVALIDATING
cases.This description was created by
for c111a42. You can customize this summary. It will automatically update as commits are pushed.