Make `clear` method fire `onChange` handlers #286

justinanastos · 2020-05-19T18:19:59Z

Resolves issue #268

Per the issue comments, I believed that the change would only require calling fireEvent.change in the clear method; but that turned out not to be the case.

React deduplicates events that it fires so that it doesn't emit multiple onChange events with the same value. Using element.value = "" causes React to expect a change event to have been fired, so calling fireEvent.change has will be considered a duplicate event and no event will be fired. I added tests to verify the expected behavior (they fail before the second commit, as one would expect).

The solution I landed on, and I'm super open to feedback, was to follow the logic I found in stackoverflow. I abstracted a setValue method that will call the native value setter, fire the change event, and then call the prototype override value setter if one exists. I tried a few other solutions that had subtle issues.

The easier solution was to call the onChange event with the target: { value: '' } and then call .value = "". This called the onChange handler, the onChange handler's event.target.value was correct, and after clear finishes, the .value would also be correct. However, if you check the original element's value inside of the onChange handler, it will be wrong because it'll be using React's prototype override; that override has an internal cache of the input value, which will be wrong. This is super subtle, but can be damning. I added a check to the tests onChange methods to make sure that event.target.value and input.value are correct when onChange is called.

Lastly, I added checks to the existing tests to make sure onChange is called when expected.

I expected this to be a much simpler fix; but this is the best I could come up with. I'd love to get some feedback!

evocateur · 2020-05-19T19:46:31Z

src/index.js

@@ -1,7 +1,7 @@
 import { fireEvent } from "@testing-library/dom";

 function wait(time) {
-  return new Promise(function (resolve) {
+  return new Promise(function(resolve) {


With all of these irrelevant formatting changes, it's really hard to grok the real diff.

Is this caused by the pre-commit hook, or is this a wonky editor plugin?

Oh how embarrassing. Let me go through and figure out why I have whitespace changes.

~~Circling back this is totally the pre-commit hook. Let me separate out the actual changes I proposed from the prettier changes.~~

justinanastos · 2020-05-19T20:50:50Z

@evocateur I separated out a commit that has the prettier updates applied by the pre-commit hook to the files I was about to change. Hopefully this makes reviewing commit-by-commit useful now. Sorry for not doing this to begin with.

kentcdodds · 2020-05-19T21:41:43Z

src/index.js


  if (!element.readOnly) {
    fireEvent.input(element, {
      inputType: "deleteContentBackward"
    });
-    element.value = ""; // when we add special keys to API, will need to respect selected range
+
+    setElementValue(element, "");


Hmmm... Is there a reason we can't just use fireEvent.change(element, {target: {value}})? The setElementValue function is basically what fireEvent.change does: https://github.com/testing-library/dom-testing-library/blob/8846eaf20972f8e41ed11f278948ac38a692c3f1/src/events.js#L81-L96

Great question. I tried to explain this in the PR description but I obviously struggled and didn't get the point across. Also, I hadn't yet seen the setNativeValue implementation. Thanks for the patience of letting me try again.

My first attempt tried to use fireEvent.change(element, { target: { value: '' }}) without realizing that they have a very similar function to the function that you linked to (https://github.com/testing-library/dom-testing-library/blob/8846eaf20972f8e41ed11f278948ac38a692c3f1/src/events.js#L81-L96) above #286 (comment).

If I replace this setElementValue( ... ) with the fireEvent.change, the problem is that React's internal representation of state will fall out of sync with the actual input.value because React internally caches the input value https://github.com/facebook/react/blob/8b3b5c35245b3aa0b2802b51856fe9dad5bcb671/packages/react-dom/src/client/inputValueTracking.js#L51-L104. When we set the native .value, the React value setter isn't called, causing React to have a different internal value than the input itself. Fortunately the test additions I added will catch this. Changing the value will show that the value is not maintained.

This is the part of my abstraction that differs from the above linked: https://github.com/testing-library/user-event/pull/286/files#diff-1fdf421c05c1140f6d71444ea2b27638R144-R148, which calls the prototype overridden value setter after the event is dispatched.

However, there's a way simpler solution that I didn't see and is now obvious after looking at the fireEvent.change and these new tests. If I just call input.value = "" after calling fireEvent.change then everything works as we'd expect! I'll make this change.

codecov · 2020-05-20T15:58:32Z

Codecov Report

Merging #286 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #286   +/-   ##
=======================================
  Coverage   98.75%   98.76%           
=======================================
  Files           1        1           
  Lines         161      162    +1     
  Branches       46       46           
=======================================
+ Hits          159      160    +1     
  Misses          2        2

Impacted Files	Coverage Δ
src/index.js	`98.76% <100.00%> (+<0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6032f14...4fb2c41. Read the comment docs.

vadimshvetsov · 2020-05-20T16:44:59Z

It looks like your prettier config overrides repo one somehow. I've made changes in some PRs literally yesterday and I haven't got this format diffs. If you can't handle this pre-commit hook, you could try to use `git commit --no-verify' flag

justinanastos · 2020-05-20T17:44:23Z

It looks like your prettier config overrides repo one somehow. I've made changes in some PRs literally yesterday and I haven't got this format diffs. If you can't handle this pre-commit hook, you could try to use `git commit --no-verify' flag

I wish I had a good explanation of what I was doing wrong @vadimshvetsov. I checked out master and ran prettier after an npm ci. I have no idea why I was seeing discrepancies before; they are gone now. Thanks for your patience.

vadimshvetsov · 2020-05-20T20:03:22Z

__tests__/react/clear.js

@@ -72,14 +78,18 @@ describe("userEvent.clear", () => {
    it.each(["input", "textarea"])(
      `should clear when <%s> is of type="${type}"`,
      (inputType) => {
+        const onChange = jest.fn().mockImplementation((event) => {
+          expect(input).toHaveProperty("value", "");
+          expect(event.target).toHaveProperty("value", "");


I'm wondering is there a specific case why assertions placed inside a mocked implementation instead of test flow? Isn't input strict equal event.target here and already checked in the end of the test (on line 100)?

Yes, input is strict equal to event.target.

I ran into an edge case with the implementation that I've since scrapped. The sequence was events was as follows:

call input's prototype value setter

fired change event (wasn't using fireEvent.change)

use input.value setter, which is overridden by React

In this case, when the event handler was fired, input.value would still be the previous value because input.value's getter is also overridden by React and uses an internal copy of the state. If we didn't use input.value = to assign the value, then the internal state would not be updated. This is super subtle and I'm fine getting rid of it, but this does add a little bit of confidence. I'll add a comment in the meantime and remove if it that's what you want.

kentcdodds · 2020-05-20T22:34:38Z

I super duper apologize here, but I just made a bunch of changes to the project to clean things up a bit and now there are some significant merge conflicts. Rather than fix the merge conflicts, it may be easier to create the PR anew.

I hope you accept my apology. I know that's totally not fun. Let me know if you'd rather I just do this myself 😬 Sorry!

justinanastos · 2020-05-21T17:36:50Z

I super duper apologize here, but I just made a bunch of changes to the project to clean things up a bit and now there are some significant merge conflicts. Rather than fix the merge conflicts, it may be easier to create the PR anew.

I hope you accept my apology. I know that's totally not fun. Let me know if you'd rather I just do this myself 😬 Sorry!

Thanks for the offer @kentcdodds ; merge conflicts resolved, rebased, and pushed.

kentcdodds

Super, thank you very much!

kentcdodds · 2020-05-21T18:43:02Z

🎉 This PR is included in version 10.3.4 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

evocateur reviewed May 19, 2020

View reviewed changes

justinanastos force-pushed the fix/clear branch from 0ac1232 to bdfdb6d Compare May 19, 2020 20:49

kentcdodds reviewed May 19, 2020

View reviewed changes

justinanastos force-pushed the fix/clear branch from bdfdb6d to 8e492dc Compare May 20, 2020 15:55

justinanastos requested a review from kentcdodds May 20, 2020 15:55

justinanastos force-pushed the fix/clear branch from 8e492dc to df49bfa Compare May 20, 2020 17:42

justinanastos marked this pull request as draft May 20, 2020 17:45

justinanastos marked this pull request as ready for review May 20, 2020 17:47

vadimshvetsov reviewed May 20, 2020

View reviewed changes

justinanastos force-pushed the fix/clear branch from df49bfa to 43db5f6 Compare May 21, 2020 17:25

justinanastos added 2 commits May 21, 2020 13:36

test: add tests to clear method to track onChange calls

e6697be

fix: fix onChange not being called in clear

825b6ae

justinanastos force-pushed the fix/clear branch from 43db5f6 to 825b6ae Compare May 21, 2020 17:36

Update index.js

4fb2c41

kentcdodds approved these changes May 21, 2020

View reviewed changes

kentcdodds merged commit bfbe92c into testing-library:master May 21, 2020

kentcdodds added the released label May 21, 2020

tgallacher mentioned this pull request May 22, 2020

userEvent.clear() does not fire onChange handlers #268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `clear` method fire `onChange` handlers #286

Make `clear` method fire `onChange` handlers #286

justinanastos commented May 19, 2020

evocateur May 19, 2020

justinanastos May 19, 2020

justinanastos May 19, 2020 •

edited

justinanastos commented May 19, 2020

kentcdodds May 19, 2020

justinanastos May 20, 2020

codecov bot commented May 20, 2020 •

edited

vadimshvetsov commented May 20, 2020

justinanastos commented May 20, 2020 •

edited

vadimshvetsov May 20, 2020

justinanastos May 21, 2020

kentcdodds commented May 20, 2020

justinanastos commented May 21, 2020

kentcdodds left a comment

kentcdodds commented May 21, 2020

Make clear method fire onChange handlers #286

Make clear method fire onChange handlers #286

Conversation

justinanastos commented May 19, 2020

evocateur May 19, 2020

Choose a reason for hiding this comment

justinanastos May 19, 2020

Choose a reason for hiding this comment

justinanastos May 19, 2020 • edited

Choose a reason for hiding this comment

justinanastos commented May 19, 2020

kentcdodds May 19, 2020

Choose a reason for hiding this comment

justinanastos May 20, 2020

Choose a reason for hiding this comment

codecov bot commented May 20, 2020 • edited

Codecov Report

vadimshvetsov commented May 20, 2020

justinanastos commented May 20, 2020 • edited

vadimshvetsov May 20, 2020

Choose a reason for hiding this comment

justinanastos May 21, 2020

Choose a reason for hiding this comment

kentcdodds commented May 20, 2020

justinanastos commented May 21, 2020

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented May 21, 2020

Make `clear` method fire `onChange` handlers #286

Make `clear` method fire `onChange` handlers #286

justinanastos May 19, 2020 •

edited

codecov bot commented May 20, 2020 •

edited

justinanastos commented May 20, 2020 •

edited