final-form / react-final-form

🏁 High performance subscription-based form state management for React
https://final-form.org/react
MIT License
7.38k stars 480 forks source link

Providing a onChange prop for <Field> should not overwrite the internal final-form onChange function #91

Open Skaronator opened 6 years ago

Skaronator commented 6 years ago

Are you submitting a bug report or a feature request?

bug report

What is the current behavior?

When passing my own onChange prop it actually overwrites the final-form onChange function.

What is the expected behavior?

When passing my own onChange function to the <Field> component it should merge with the internal onChange function of final-form

Sandbox Link

https://codesandbox.io/s/w7pl6pqkrw

Try to select a value in the second dropdown. It'll fire the custom myOnChange function but won't select the value because it overwrites the final-form onChange function.

What's your environment?

"final-form": "^3.0.0",
"react-final-form": "^2.1.1",
"react": "16.2.0",
"react-dom": "16.2.0",
node version: v8.9.1
felixjung commented 6 years ago

Hi,

Thanks for submitting the issue and providing an example. I don't have any working knowledge of the library, yet, other than reviewing the docs. However, looking at your suggestion, I don't think this is a valid request. How would you expect the library to know how to compose (merge) your myOnChange with its own onChange? And, given that you basically overwrite the default onChange, I don't think it should. But, your problem can be solved by using the render prop API for fields and providing your own onChange handler.

I've updated your example with the solution I have in mind: https://codesandbox.io/s/3qlx8o98j5

To the maintainers: this piece of software looks awesome and the docs are great! Thanks a lot!

felixjung commented 6 years ago

After reading the contribution guidelines, I think is was more of a question and would probably deserve a StackOverflow question.

Skaronator commented 6 years ago

Instead of overwriting the internal onChange function it could merge the onChange prop with the internal function. I don't know anything about this library code yet but I assume this should be possible.

Thanks for the Sandbox example I'm already doing that similar to your approache:

const CustomSelect = ({
  input: { name, onChange, ...inputRest },
  meta: { touched, error, ...metaRest },
  label,
  children,
  className,
  onCustomChange,
  ...props
}) => {
  const isError = Boolean(touched && error);
  return (
    <FormControl className={className} error={isError}>
      <InputLabel htmlFor={name}>{label}</InputLabel>
      <Select
        input={<Input name={name} id={name} />}
        onChange={e => {
          onChange(e);
          onCustomChange(e);
        }}
        {...inputRest}
        {...props}
      >
        {children}
      </Select>
      {isError && <FormHelperText>{error}</FormHelperText>}
    </FormControl>
  );
};
mschipperheyn commented 6 years ago

IMHO this is a totally valid request. I consider this a bug.

maciejmyslinski commented 6 years ago

@erikras I'd consider this a bug since the documentation doesn't mention such behavior. I suggest either updating documentation or implementing a fix for it. Could you please give me guidance, so that I can start working on it? ✌️

ephraimt-welldone commented 5 years ago

I consider this a bug. Definitely need this behavior fixed.

larissa-n commented 5 years ago

@erikras what's the status of this bug? Thank you!