Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

supra08
Copy link

@supra08 supra08 commented May 13, 2025

Important

Enhances useField's onChange to accept non-string values and fixes a formatting issue in formikReducer.

  • Behavior:
    • executeChange in Formik.tsx now accepts string, React.ChangeEvent, or any type, allowing non-string values.
    • onChange in FieldInputProps in types.tsx updated to accept React.ChangeEvent, field value type, or string.
  • Bug Fix:
    • Fixes formatting issue in formikReducer in Formik.tsx by separating SET_ISSUBMITTING and SET_ISVALIDATING cases.

This description was created by Ellipsis for c111a42. You can customize this summary. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a 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 in 2 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% <= threshold 50% 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 Ellipsis 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':
Copy link

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') {
Copy link

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.

Suggested change
if (!isString(eventOrTextValue) && !Array.isArray(eventOrTextValue) && typeof eventOrTextValue !== 'number' && typeof eventOrTextValue !== 'boolean' && typeof eventOrTextValue !== 'object') {
if (eventOrTextValue && (typeof eventOrTextValue === 'object') && (eventOrTextValue.target || eventOrTextValue.persist)) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant