nkbt / react-debounce-input

React component that renders Input with debounced onChange
MIT License
451 stars 61 forks source link

event.currentTarget is null #61

Open aaronbeall opened 7 years ago

aaronbeall commented 7 years ago

When handling the onChange event the currentTarget is null.

You can see an example on webpackbin here:

<DebounceInput
  minLength={2}
  debounceTimeout={300}
  onChange={event => {
    console.log("target =", event.target);
    console.log("target.value =", event.target.value);
    console.log("currentTarget =", event.currentTarget);
   }} />

Which gives you the following output after typing "test":

target = <input type=​"text" value data-reactid=​".0.0">​ target.value = test currentTarget = null

I take it this is because the event is stored then re-dispatched later, and React nulls out the currentTarget after the initial dispatch. However this is not what I would expect because its not how the event argument normally works, you expect currentTarget to be the target you added the handler to.

nkbt commented 7 years ago

I don't think this is fixable outside of react. I mean, I would prefer not to patch over react's event to normalize behavior. Also this makes sense since between the fact of event and event re-dispatch target might be removed (not the case here, but I see the reasining behind nulling it) On Wed., 7 Dec. 2016 at 16:13, Aaron Beall notifications@github.com wrote:

When handling the onChange event the currentTarget is null.

You can see an example on webpackbin here http://www.webpackbin.com/41fzSFgmf:

<DebounceInput minLength={2} debounceTimeout={300} onChange={event => { console.log("target =", event.target); console.log("target.value =", event.target.value); console.log("currentTarget =", event.currentTarget); }} />

Gives you the output after typing "test":

target = <input type=​"text" value data-reactid=​".0.0">​ target.value = test currentTarget = null

I take it this is because the event is stored then re-dispatched later, and React nulls out the currentTarget after the initial dispatch. However this is not what I would expect because its not how the event argument normally works, you expect currentTarget to be the target you added the handler to.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nkbt/react-debounce-input/issues/61, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKsoFZNzs0GL2jefbBY-NPfrqiE-oTRks5rFkCBgaJpZM4LGNH6 .

aaronbeall commented 7 years ago

Seems reasonable, but the event argument is already patched in some cases with {target: {value}}. I would say that given the way this works the onChange callback with an event arg is a little misleading, since it receives a stale event argument. Maybe it makes more sense to just invoke with your own argument, ie onChange(newValue) or onChange(newValue, lastEvent). That's a breaking API change so I'm not expecting you to do this, but that's just my thought. :)

nkbt commented 7 years ago

It is how it was before (prev major version), but this component is now mostly used as drop-in replacement for inputs, so current API is something we are quite happy with. Since we do not do mutable DOM operations, we never used either target or any other attribute of event like currentTarget and I would prefer not to do modifications to the original React event (updating target.value is an essential and naturally expected patch)

Maybe adding something about it to the README would be good, tho. If you are keen to PR...

aaronbeall commented 7 years ago

Yes I like that it's mostly a drop-in replacement for <input>. This was just a gotcha I struggled with.

So just trying to understand here, you say you never used target? How should you query the changed value then?

nkbt commented 7 years ago

I meant target DOM, obviously we do onChange({target: {value}}) {this.setState({value})} or smth similar in any other code. Just never touch the DOM.

aaronbeall commented 7 years ago

OK, sorry not clear how the DOM relates, patching the React event { currentTarget: { value } } seems like the same thing as patching { target: { value } } to me, but it would be needed in all cases which I understand is more intrusive, and event.target.value is a clean workaround. A simple mention about this in docs sounds fair to me.

BTW, the reason I ran into this is because the React event interface in TypeScript specifies the type of currentTarget (because that type is known based on what the handler is attached to, an HTMLInputElement in this case) while the target cannot be predicted due to bubbling. So you can either use currentTarget.value because .value is a known prop of the current target (the input) or use a type assertion (target as HTMLInputElement).value. Usually you want currentTarget but this is an atypical case.