jaredpalmer / formik

Build forms in React, without the tears 😭
https://formik.org
Apache License 2.0
33.9k stars 2.79k forks source link

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

Open shaunaas opened 1 year ago

shaunaas commented 1 year ago

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 (

); }; `

<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

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 :open_mouth: in the remaining props in the case there's no className… My vote is on reverting this PR.

HansAarneLiblik commented 1 year ago

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

My suggestion is also to revert this

colin-oos commented 1 year ago

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