Skip to content
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

2.4.4 is a Breaking Change - passing "className" this new way overwrites existing "className" in composed components #3883

Open
shaunaas opened this issue Sep 12, 2023 · 6 comments · May be fixed by #3884

Comments

@shaunaas
Copy link

shaunaas commented Sep 12, 2023

Bug report

Current Behavior

2.4.4 unexpectedly broke all of my fields.
The way the code is implemented, className becomes required and overwrites all my classNames that exist on my composed components being passed into <Field component={MyCustomComponent} />

Expected behavior

className does not become a required prop

Reproducible example

const CustomInput = (props) => { return ( <div> <input type="text" className="composed-component-classname" {...props} /> </div> ); };

<Field name="name" component={CustomInput} />

Suggested solution(s)

  1. Revert this change. Passing "className" was already supported via ...rest props and a component that either spreads/or specifically requires any specific props to be passed.

Additional context

  • passing className after the ...rest in the formik lib will overwrite any existing className prop already on the input

Your environment

Software Version(s)
Formik 2.4.4
React 18.2.0
TypeScript N.A.
Browser Any
Yarn 1.22.19
Operating System macOS 13.5.1 (Ventura)
@myNameIsDu
Copy link

myNameIsDu commented Sep 12, 2023

This #3862 caused this problem.

But I think you can make your component safer and clearer. like this

const CustomInput = ({propsA, propsB}) => { return ( <div> <input type="text" className="composed-component-classname"   propsA={propsA}  propsB={propsB}/> </div> ); };

Probably you have written props type to make your code look clearer, but either way {...props} is not a safe style of writing, because if Formik add a type props one day, it will break your code

@shaunaas
Copy link
Author

The above is an example for simplicity.. not actual code.

1 - A breaking change should not be put into a minor release.
2 - Spread props are explicitly intended to work exactly like the above example. The lib should have a clear API - as consumers of the library may have many different use cases. There is no reason to add "type" or "className" as they were already covered by the spread ...rest props.

@myNameIsDu
Copy link

The above is an example for simplicity.. not actual code.

1 - A breaking change should not be put into a minor release. 2 - Spread props are explicitly intended to work exactly like the above example. The lib should have a clear API - as consumers of the library may have many different use cases. There is no reason to add "type" or "className" as they were already covered by the spread ...rest props.

Understood, I know that's a breaking change. But I mean you can do this to improve your code health, Anyway, it's just my personal thought. if you don't like it, I'm sorry bro, You can ignore it.

@gjvoosten
Copy link

FWIW it also breaks our code (specifically field validation, where for complex fields we add the is-invalid class), because the referenced PR now explicitly passes className=undefined 😮 in the remaining props in the case there's no className
My vote is on reverting this PR.

@HansAarneLiblik
Copy link

Also hit us. Happy we caught this with snapshottests before going live.

My suggestion is also to revert this

@colin-oos
Copy link

colin-oos commented Oct 2, 2023

This is a issue for us, especially with using tailwind. Surprised this made it into a stable minor release, I imagine this effects a lot of people.

Also, if anyone else is wondering, this actually broke for me in 2.4.3, not 2.4.4

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

Successfully merging a pull request may close this issue.

5 participants