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

Working with react-hook-form #18269

Closed
LeoLozes opened this issue Nov 8, 2019 · 29 comments

Comments

@LeoLozes
Copy link

@LeoLozes LeoLozes commented Nov 8, 2019

I'm trying to do a simple form with react-hook-form and material-ui. I'd like to be able to:

  1. submit / validate a form
  2. reset the form correctly
  3. load data correctly

Sorry, bad issue search, it looks like a duplicate of #17018. Feel free to close this.

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The first step works correctly.
The reset function clears the value, but the state of the material-ui TextField seems to stay at "filled", and the styles don't match with the cleared data.
The reset function with initial values set correctly the value, but the behavior is the same as with reset (well, the opposite in this case), the data is loaded but the internal state of the TextField is not "filled".

Expected Behavior 🤔

The material-ui input should refresh its internal state when data is loaded / cleared

Steps to Reproduce 🕹

You can try the three buttons on this CodePen:
https://codesandbox.io/s/material-demo-ywutu

  1. Submit: works correctly validating the data.
  2. Correctly resets but the state of the TextFields is not empty.
  3. Correctly loads data but the state of the TextFields is not filled.

I filed an issue at the react-hook-form project but the owner told me to open one here as well, so here we are :)

@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Nov 8, 2019

I have added the waiting for users upvotes tag. I'm closing the issue as we are not sure people are looking for such support. So please upvote this issue if you are. We will prioritize our effort based on the number of upvotes.

https://npm-stat.com/charts.html?package=formik&package=react-hook-form&package=react-final-form

@stunaz

This comment has been minimized.

Copy link
Contributor

@stunaz stunaz commented Nov 9, 2019

much needed 🙏🙏🙏 any workaround?

@sessionboy

This comment has been minimized.

Copy link

@sessionboy sessionboy commented Nov 12, 2019

much needed!

@jgb-solutions

This comment has been minimized.

Copy link

@jgb-solutions jgb-solutions commented Nov 19, 2019

Definitely needed.

@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Nov 19, 2019

Any idea of what the solution would be?

@jgb-solutions

This comment has been minimized.

Copy link

@jgb-solutions jgb-solutions commented Nov 19, 2019

I'm not sure about something specific but I know I started using both libraries and I really would like to see tighter integrations between the two. React Hook Form and Material UI are just great together. Their support for TypeScript is also nice.

@bluebill1049

This comment has been minimized.

Copy link

@bluebill1049 bluebill1049 commented Nov 19, 2019

Hey, @oliviertassinari thanks for taking a look at it. Looks like MUI TextField is working better with native input's API now! That's really nice. We used to have the place holder text overlay on top of the text when you invoke e.target.value = 'xxx'. 👍

Screen Shot 2019-11-20 at 10 06 26 am

https://codesandbox.io/s/react-hook-form-conditional-fields-delete-1frsm
In the example above <Switch /> didn't reset after reset API invoked.

It would be really nice if we can have all form related components to support native form input API (set and reset value via input's reference ) I do understand it's a large piece of work and to support a small lib like react-hook-form is something to be considered ( maybe until if the lib getting more popular ❤️)

again I do appreciate your time and effort to look into this issue (maintain a HUGE open source project like MUI is crazy to me and you are doing an amazing job to the React community). On top of that, let me know if anything I can work on my end to work with MUI too.

Cheers
Bill

@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Nov 20, 2019

@bluebill1049 Thanks for looking at it. The library seems to get a good traction, but it's still far behind formik. Is the switch reset the only issue we have?

@bluebill1049

This comment has been minimized.

Copy link

@bluebill1049 bluebill1049 commented Nov 20, 2019

thanks, @oliviertassinari for reopening and looking into this issue. 🙏 yes, you are right react-hook-form is still a new boy in the town and you don't have act if you would like to wait some more time to see if more users adopting react-hook-form, which I think it's totally reasonable (even tho inside my heart I am desperate to want it working with MUI, hahaha been selfish😼). In the meantime, I will create a table of inputs that is compatible with react-hook-form out the box (maybe even with a codesandbox too) I will post it in here as well.

If things work well with react-hook-form and usage growing bigger, maybe we can consider to working with native input API (which is what react hook form use behind the scenes) it would be great to get input, select, radio and checkbox working as those are the primary usage for form.

@bluebill1049

This comment has been minimized.

Copy link

@bluebill1049 bluebill1049 commented Nov 21, 2019

FYI @oliviertassinari I am working on something to close the gap between MUI and react-hook-form. https://github.com/react-hook-form/react-hook-form-input

@raikusy

This comment has been minimized.

Copy link

@raikusy raikusy commented Nov 23, 2019

Also facing issue with reset and loading default data. Please look into this issue. Or at least provide a work around for now.

@bluebill1049

This comment has been minimized.

Copy link

@bluebill1049 bluebill1049 commented Nov 23, 2019

Hey @raikusy please take a look the link which I have posted above, i think it will help you out. In the long run it would would be great for MUI component to support native form api ‘reset’ and update value via component ‘ref’, but this is a large price of work which I am sure MUI have a lot features and ideas lined up which are more important. Just as we discussed above until react hook form gets to be the mainstream lib or become popular then MUI will start investigating for solutions. Hope these make sense :) thanks for using react hook form too ❤️🤩🤟🏻

FYI that wrapper component will still enable ur form to have 0 re-render. Because input state update is isolated within the wrapper component.🤩🤟🏻💃

@raikusy

This comment has been minimized.

Copy link

@raikusy raikusy commented Nov 23, 2019

Thank you @bluebill1049 this looks great. I have a condition where form fields will have default value when data is passed from props. So will the TextField component work with the above solution too? Also I was looking for any better example of passing default values with props to the form. The props might sometime come from a api call after the form has already been mounted.

@bluebill1049

This comment has been minimized.

Copy link

@bluebill1049 bluebill1049 commented Nov 23, 2019

@raikusy you should be able to use reset/setValue API with the wrapper compoent.
reset: for entire form -> https://react-hook-form.com/api#setValue
setValue: for individual - > https://react-hook-form.com/api#reset

@hbarcelos

This comment has been minimized.

Copy link

@hbarcelos hbarcelos commented Nov 27, 2019

I cannot get <Slider /> to work with RHF.

Using it like:

      <FormControl fullWidth component="fieldset" margin="normal">
        <FormLabel component="legend">Palavras</FormLabel>
        <RHFInput
          as={<Slider min={100} max={1200} step={100} valueLabelDisplay="auto" marks={marks} />}
          type="input"
          name="words"
          register={register}
          setValue={setValue}
        />
      </FormControl>

I get the following error:

Warning: Failed prop type: Invalid prop `value` supplied to `ForwardRef(Slider)`.
    in ForwardRef(Slider) (created by WithStyles(ForwardRef(Slider)))
    in WithStyles(ForwardRef(Slider)) (created by SetupAccountForm)
    in Unknown (created by SetupAccountForm)
    in fieldset (created by ForwardRef(FormControl))
    in ForwardRef(FormControl) (created by WithStyles(ForwardRef(FormControl)))
    in WithStyles(ForwardRef(FormControl)) (created by SetupAccountForm)
    in form (created by SetupAccountForm)
    in SetupAccountForm (created by SetupAccountPage)
    in div (created by ForwardRef(CardContent))
    in ForwardRef(CardContent) (created by WithStyles(ForwardRef(CardContent)))
    in WithStyles(ForwardRef(CardContent)) (created by SetupAccountPage)
    in div (created by ForwardRef(Paper))
    in ForwardRef(Paper) (created by WithStyles(ForwardRef(Paper)))
    in WithStyles(ForwardRef(Paper)) (created by ForwardRef(Card))
    in ForwardRef(Card) (created by WithStyles(ForwardRef(Card)))
    in WithStyles(ForwardRef(Card)) (created by SetupAccountPage)
    in div (created by ForwardRef(Grid))
    in ForwardRef(Grid) (created by WithStyles(ForwardRef(Grid)))
    in WithStyles(ForwardRef(Grid)) (created by Layout)
    in Layout (created by SetupAccountPage)
    in SetupAccountPage (created by App)
    in Route (created by App)
    in Switch (created by App)
    in Router (created by HashRouter)
    in HashRouter (created by App)
    in App (created by Root)
    in div (created by ForwardRef(Container))
    in ForwardRef(Container) (created by WithStyles(ForwardRef(Container)))
    in WithStyles(ForwardRef(Container)) (created by Root)
    in div (created by Styled(MuiBox))
    in Styled(MuiBox) (created by Root)
    in Provider (created by Root)
    in Root Button.js:233:15
@bluebill1049

This comment has been minimized.

Copy link

@bluebill1049 bluebill1049 commented Nov 27, 2019

@hbarcelos looks like slider doesn't support value props :( I will do some investigation on it. in the meantime, you may have to use custom register, which is register at useEffect

@hbarcelos

This comment has been minimized.

Copy link

@hbarcelos hbarcelos commented Nov 27, 2019

Thanks a lot @bluebill1049

I ended up making something like this:

import React, { useEffect, useCallback, useState, useMemo } from 'react';
import t from 'prop-types';
import clsx from 'clsx';
import { makeStyles } from '@material-ui/core/styles';
import Box from '@material-ui/core/Box';
import Slider from '@material-ui/core/Slider';

const useStyles = makeStyles(theme => ({
  sliderWrapper: {},
  sliderLabel: {
    marginTop: theme.spacing(1),
    marginBottom: theme.spacing(0.5),
    color: theme.palette.primary.main,
  },
}));

function CustomSlider({
  register,
  unregister,
  setValue,
  name,
  rules,
  defaultValue,
  valueLabelAs,
  formatLabel,
  ...rest
}) {
  const cl = useStyles();

  const [currentValue, setCurrentValue] = useState(defaultValue);

  useEffect(() => {
    register({ name });
    return () => unregister(name);
  }, [defaultValue, name, register, setValue, unregister]);

  const valueLabel = useMemo(() => {
    if (!valueLabelAs) {
      return null;
    }

    return React.cloneElement(
      valueLabelAs,
      { className: clsx(valueLabelAs.props.className, cl.sliderLabel) },
      formatLabel(currentValue)
    );
  }, [cl.sliderLabel, currentValue, formatLabel, valueLabelAs]);

  const handleChange = useCallback(
    (_, value) => {
      setValue(name, value);
      setCurrentValue(value);
    },
    [name, setValue]
  );

  return (
    <React.Fragment>
      {valueLabel}
      <Box className={cl.sliderWrapper}>
        <Slider {...rest} onChange={handleChange} defaultValue={defaultValue} />
      </Box>
    </React.Fragment>
  );
}

CustomSlider.defaultProps = {
  rules: {},
  defaultValue: '',
  valueLabelAs: null,
  formatLabel: v => v,
};

CustomSlider.propTypes = {
  register: t.func.isRequired,
  unregister: t.func.isRequired,
  setValue: t.func.isRequired,
  name: t.string.isRequired,
  rules: t.object,
  defaultValue: t.oneOfType([t.number, t.string]),
  valueLabelAs: t.node,
  formatLabel: t.func,
};

export default CustomSlider;

It works as expected.

@lednhatkhanh

This comment has been minimized.

Copy link

@lednhatkhanh lednhatkhanh commented Dec 7, 2019

Much needed this also

@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Dec 7, 2019

In the long run it would would be great for MUI component to support native form api ‘reset’ and update value via component ‘ref’.

@bluebill1049 Isn't this an issue with React or react-hook-form itself? How is a developer supposed to "react" to an input state change with react-hook-form?
Let's say I'm a React developer building a custom input, and I want to style the element differently when it's filled/empty, how should I implement this? It seems that no change event is ever fired.

Have you considered firing a change event when your library changes an input value "outside" React's declarative model? Like it's done in @test-library/react?

This would enable the support of Material-UI components that rely on native input elements, for the others, like the Slider, a custom wrapper would still be needed. The slider has a value prop but the signature of onChange is different from a native input. I think that it would be great to see an integration library as we have for the other form libraries: #15585.


I was wondering why this issue got so many upvotes, I think that I have found out 🙃:

Capture d’écran 2019-12-07 à 20 41 31
https://react-hook-form.com/advanced-usage#ControlledmixedwithUncontrolledComponents

@bluebill1049

This comment has been minimized.

Copy link

@bluebill1049 bluebill1049 commented Dec 7, 2019

hey @oliviertassinari, thanks for looking closer to this issue again 👍

Let's say I'm a React developer building a custom input, and I want to style the element differently when it's filled/empty, how should I implement this? It seems that no change event is ever fired.

I would seek CSS selector for solutions in my own project which is using native inputs, eg: select an element has an empty value and display a style. An alternative I would use react-hook-form watch API to detect empty value and pass down as prop (with my wrapped input component).

Have you considered firing a change event when your library changes an input value "outside" React's declarative model? Like it's done in @test-library/react?

Yeah, we are pretty much doing it under react-hook-form-input. 😄

I believe in uncontrolled components for building forms after many years of building form controlled, it really makes things much easier. It may not solve every edge case (if you working at Facebook LOL) but it certainly makes my work life and others around me easier when it comes to building forms. HTML inputs are stateful themselves and I would love to embrace them in this lib (not saying it's right or wrong, but an alternative solution).

I love building React application so much, and that's why building a lot of packages around it. I understand React to embrace controlled components. However, react-hook-form is not blocking developers from building form controlled, because you can still do custom register at useEffect.

Conclusion:

I think we can close this issue and I will remove the link on my website related to this issue. On top of that, could I update this page to include react-hook-form-input?

https://material-ui.com/components/text-fields/#complementary-projects

@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Dec 7, 2019

Yeah, we are pretty much doing it under react-hook-form-input.

@bluebill1049 It sounds like the opposite approach to my suggestion. I understand the following:

  • react-hook-form:
    1. It keeps track of the input DOM nodes. When it needs to change the value, it does input.value = 'x'. This is problematic for React, as it doesn't have any way to know that the input value has changed. For instance, no change event is triggered.
    2. Because react-hook-form needs to listen for input value changes, it sets an "input" event listener on the input. This is problematic for your library as changes by React doesn't trigger any event when the value changes from a controlled input.
  • react-hook-form-input: the library controls the input to work around the two previous limitations of react-hook-form (i and ii).

Given that 1. it's very unlikely that React will play nicely to support react-hook-form approach and 2. that react-hook-form doesn't work outside of the box with controlled and pseudo-uncontrolled inputs (the ones that change the default value and the ones that only listen to onChange), I would propose that:

  1. react-hook-form patches the controlled and pseudo uncontrolled problem internally. Basically, you need to solve i. and ii. (see the proposed solutions in the linked codesandboxes: dispatch + defineProperty).
  2. Material-UI seeks assistance from the community to provide adapters for react-hook-form, when needed (related to #15585).
  3. We close this issue 👌
@bluebill1049

This comment has been minimized.

Copy link

@bluebill1049 bluebill1049 commented Dec 7, 2019

thanks, @oliviertassinari for the detailed reply. I will do some further investigation with your proposed solutions :)

I believe most of the issue you mentioned above with React is when you switch controlled components which is the part I want to improve on.

@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Dec 7, 2019

Just to be clear:

So I'm pretty sure you can solve that in react-hook-form. We would appreciate it if you can try/deploy these fixes and remove the blame on our side from your documentation 😛.

These changes should really help with the traction of your form library, I hope you will appreciate it :D.

@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Dec 7, 2019

I believe most of the issue you mentioned above with React is when you switch from uncontrolled to controlled or controlled components which is the part I want to improve on.

React warns when the user switches between uncontrolled and controlled. I don't understand your point.

@bluebill1049

This comment has been minimized.

Copy link

@bluebill1049 bluebill1049 commented Dec 7, 2019

So I'm pretty sure you can solve that in react-hook-form. We would appreciate it if you can try/deploy these fixes and remove the blame on our side from your documentation 😛.

NO WAY I am blaming MUI haha, I love MUI (with my little star ⭐️). As you would know, react-hook-form is a new boy in the town, I was trying to get some attention or momentum on the particular issue. I apologize if you find that way (blame). again thanks very much for your help and investigation.

image
(Not Blaming)

@bluebill1049

This comment has been minimized.

Copy link

@bluebill1049 bluebill1049 commented Dec 7, 2019

I believe most of the issue you mentioned above with React is when you switch from uncontrolled to controlled or controlled components which is the part I want to improve on.

React warns when the user switches between uncontrolled and controlled. I don't understand your point.

sorry I meant controlled component :) fixed my comment

@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Dec 7, 2019

@bluebill1049 Awesome :)

@bluebill1049

This comment has been minimized.

Copy link

@bluebill1049 bluebill1049 commented Dec 7, 2019

thanks a lot, @oliviertassinari (you are very kind)

@oliviertassinari

This comment has been minimized.

Copy link
Member

@oliviertassinari oliviertassinari commented Dec 7, 2019

Note that Material-UI could apply the same proposed fixes but I don't think that we should, it would have two drawbacks: 1. it would only work with Material-UI, the same effort would need to be done over and over. 2. the ownership of these "hacks" should stay in react-hook-form as it originates from the tradeoff the library took (bypassing the idiomatic React API).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
You can’t perform that action at this time.