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

React.FormEventHandler<HTMLElement> or React.ChangeEvent for Input? #3372

Closed
havesomeleeway opened this issue Feb 20, 2019 · 6 comments
Closed

Comments

@havesomeleeway
Copy link

havesomeleeway commented Feb 20, 2019

Environment

  • Package version(s):
    "@blueprintjs/core": "^3.11.0",
  • Browser and OS versions:
    Google Chrome: Version 71.0.3578.98 (Official Build) (64-bit)
    OS: Mojave 10.14.2

Question

Over at the Text Input Component,
it is stated that the props for onChange should be:

React.FormEventHandler<HTMLElement>
Change event handler. Use event.target.value for new value.

Inherited from IControlledProps.onChange

However when I try to use React.FormEventHandler for my handleChange function, this shows up:

image

There's a vague answer on stackoverflow which describes

ChangeEvent more suitable for typing form events

However, it does not explain why ChangeEvent is more suitable for typing form events.

Using React.ChangeEvent, the warning goes away. I am now able to edit my input fields on the UI.
image

My questions are:

  1. Should I use ChangeEvent or FormEventHandler for my input?
  2. If it is FormEventHandler as suggested by the documentation on Blueprint, how do I deal with the warning
    TS2339: Property 'target' does not exist on type '(event: FormEvent<HTMLInputElement>) => void'.
  3. If input fields are supposed to use React.ChangeEvent, should we update IControlledProps.onChange?

My component code is straight forward

          <FormGroup helperText={props.helperText} intent="danger">
            <InputGroup
              value={props.email}
              onChange={props.onChange}
              name="email"
              /**
               * using type of "email" instead of "text"
               * Doing so constraints the value to a syntactically valid email address.
               * This generally has the format username@hostname.tld.
               * */
              type="email"
              placeholder="Enter e-mail address"
              large
              required
            />
          </FormGroup>
@havesomeleeway
Copy link
Author

Update:

I haven't been able to get FormEventHandler working for my Input component so I'm currently using ChangeEvent. I asked around and was told that FormEventHandler was more suited for onSubmit events rather than onChange.

@jacekjagiello
Copy link
Contributor

Hey, @havesomeleeway I think there is one misconception you have - FormEventHandler does not refers to the event itself, but it refers to event handler - the function that handles this event.
Try this out:

handleChange: React.FormEventHandler<HTMLInputElement> = (event) => {
        this.setState({ [event.currentTarget.name]: event.currentTarget.value });
};

Note, that type React.FormEventHandler<HTMLInputElement> describes a handleChange function which is your event handler. In this case, Typescript will automatically recognize event parameter as React.FormEvent<HTMLInputElement> - which is the type you are looking for. Alternatively, you can describe only the event:

handleChange = (event: React.FormEvent<HTMLInputElement>) => {
        this.setState({ [event.currentTarget.name]: event.currentTarget.value });
};

But I'll recommend you to go with first solution.

Also, note that you should use currentTarget instead of target - when typescript will recognize the type of event, it will actually warn you that name/value does not exist on type EventTarget which is the type of event.target

@havesomeleeway
Copy link
Author

At work at the moment but i'll have a deeper look and thought into your answer once I head home this evening. Thanks!

@havesomeleeway
Copy link
Author

@jacekjagiello Why would you recommend the first solution as opposed to the second one from a technical point of view?

@jacekjagiello
Copy link
Contributor

Actually, according to React.InputHTMLAttributes interface which InputGroup extends, you should use React.ChangeEventHanlder or React.ChangeEvent. I've missed that in first response. React.FormEventHadnler is meant to be used for form specific events like onSubmit.

Why would you recommend the first solution as opposed to the second one from a technical point of view?

I think as a general rule it's better to add type to the whole function, rather than to individual arguments. This empowers reusability and pays off when you have a function that has multiple arguments.
A good example of benefits can be found in @blueprintjs/select package. Consider this:

interface Item {
    label: string
    value: string
}

class App extends React.PureComponent<{}, {}> {
   render() {
       const items: Item[] = [
           { label: 'First item', value: 'first' },
           { label: 'Second item', value: 'second' },
           { label: 'Third item', value: 'third' }
       ]
       return <Select items={items} itemRenderer={this.renderItem} onItemSelect={console.log} />
   }

   // type for function
   // - no need to specify type for each of the arguments 
   // - you know execaly the type of renderItem function - more reable, better for docs
   // - reusable, you can add ItemRenderer to other functions, and they reamin consistent to single type
   private renderItem: ItemRenderer<Item> = (item, itemProps) => {
       return <MenuItem text={item.label} onClick={itemProps.handleClick} />
   }
   
   // type only for argument
   // need to specify type for each of the arguments
   private renderItem = (item: Item, itemProps: IItemRendererProps) => {
        return <MenuItem text={item.label} onClick={itemProps.handleClick} />
   }
}

@havesomeleeway
Copy link
Author

https://www.typescriptlang.org/docs/handbook/functions.html

Had a good read of this and I better understand what you are saying! Thanks!

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

No branches or pull requests

2 participants