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

<Select /> onChange handler does not trigger in jsdom #596

Closed
tstirrat opened this issue Apr 3, 2020 · 12 comments
Closed

<Select /> onChange handler does not trigger in jsdom #596

tstirrat opened this issue Apr 3, 2020 · 12 comments

Comments

@tstirrat
Copy link

tstirrat commented Apr 3, 2020

  • What RMWC Version are you using [major.minor.patch]: 6.0.9

  • Name your build system [Webpack, Rollup...]: CRA 2.0

  • Describe the bug with as much detail as possible:

Select onChange has changed in 6.0 to a CustomEvent, where it was previously (in 5.7.2) a SyntheticEvent.

Things appear to work fine in browsers:
5.7.2: https://codesandbox.io/s/rmwc-572-select-events-kbvlz
6.0.9: https://codesandbox.io/s/rmwc-609-select-events-pbrw3 (note also that the inputRef= never returns a ref too)

however in jsDOM environments, any tests that rely on triggering 'change' fail.

  • What happened, and what was supposed to happen:

In a jest test, fireEvent.change(submitEl, ...) would trigger onChange in 5.7.2

In 6.0.x, it does not trigger onChange.

You can see a minimal repro here:

5.7.2 - tests pass: https://github.com/tstirrat/rmwc-select-change-event/tree/working-5.7.2
6.0.9 - test fails: https://github.com/tstirrat/rmwc-select-change-event (master branch)

Also note that in 6.0.9 the label is not enclosed in a <label>, so I had to do some weird getByText dom traversal.

@tstirrat
Copy link
Author

tstirrat commented Apr 3, 2020

I cannot find a way to sanely simulate a change event in jsdom tests btw.

I've tried Simlate.change.

I notice that in 6.0, CustomEvent type is onChange and not SyntheticEvent of change too.

I even tried fireEvent(element, new CustomEvent('onChange')) and this did nothing, too.

@jamesmfriedman
Copy link
Collaborator

MDC rewrote the select (again) and changed the markup. Thank you for the repro’s, I’ll get these fixed and look into the onChange.

@tstirrat
Copy link
Author

tstirrat commented Apr 4, 2020

Great, for now I've kept select at 5.7.2 and the rest at 6.0.9 in our app

@jamesmfriedman
Copy link
Collaborator

@tstirrat dug into this for a bit. It's caused by an issue that had to be worked around in Google's code. Effectively, material-components-web fires the onChange handler on init. This definitely is NOT what we want, since it is contrary to how standard selects behave in React. Because of this, I had to employ a workaround to prevent onChange events from happening immediately on mount.

For the time being, here is a workaround which just involves delaying your onChange event firing.

test('renders learn react link', (done) => {
  const onChange = jest.fn();
  const { container } = render(<Select onChange={onChange} />);

  const select = container.children[0].querySelector('select');

  act(() => {
    window.requestAnimationFrame(() => {
      fireEvent.change(select!);
      expect(onChange).toHaveBeenCalled();
      done();
    });
  });
});

Let me know if that works for you. Ideally I could fix this and make it behave exactly like a native select, but due to the complicated nature of the integration with material-components-web under the hood I don't know if it would be possible.

@tstirrat
Copy link
Author

tstirrat commented Apr 4, 2020

Thanks, I'll try that!

@tstirrat
Copy link
Author

tstirrat commented Apr 6, 2020

This workaround did work, thanks!

@tstirrat
Copy link
Author

tstirrat commented Apr 6, 2020

Unfortunately, it's not completely working. I am using an async rAF delay to fire the event. this works, it fires the onChange. but in testing, it doesn't include the select's values:

    expect(jest.fn()).toHaveBeenCalledWith(...expected)

    Expected: "false"
    Received: "true"

    Number of calls: 1

      33 |
      34 |   expect(onChange).toHaveBeenCalledTimes(1);
    > 35 |   expect(onChange).toHaveBeenCalledWith('false');
         |                    ^
      36 | });
      37 |

      at Object.<anonymous> (src/App.test.js:35:20)

  console.log src/App.test.js:10
    onChange CustomEvent { isTrusted: [Getter] }.  <-------------- no (target|currentTarget).value

Test Suites: 1 failed, 1 total

Code updated here using the latest 6.0.14: https://github.com/tstirrat/rmwc-select-change-event

@tstirrat
Copy link
Author

tstirrat commented Apr 6, 2020

Using async firing:

  await act(() => {
    return new Promise((resolve) => {
      requestAnimationFrame(() => {
        fireEvent.change(select, {
          target: { value: 'false' },
        });
        resolve();
      });
    });
  });

  expect(onChange).toHaveBeenCalledTimes(1);
  expect(onChange).toHaveBeenCalledWith('false');

@jamesmfriedman
Copy link
Collaborator

Well, I hate that you're having these issues. Did a little more investigating

  • Internally ,RMWC uses the "selectedIndex" property of the DOM event
  • I assumed that passing in the appropriate like so would have fixed the issue
await act(() => {
    return new Promise((resolve) => {
      requestAnimationFrame(() => {
        fireEvent.change(select, {
          currentTarget: { selectedIndex: 1 },
        });
        resolve();
      });
    });
  });

That doesn't appear to get us any closer, but it seems like anyone would have this exact same issue if they're using the select change in a unit test, so I'd really like to figure it out.

@tstirrat
Copy link
Author

tstirrat commented Apr 6, 2020

Thanks for investigating more. Yes I think it will surface with others too.

@astrofrans
Copy link

astrofrans commented Jun 15, 2020

Perhaps a scope creep on this ticket, but I'm having issues with the onChange for the Select field too since upgrading to v.6.x.y , but in MSIE11.

The target and customTarget props are not copied correctly into the CustomEvent in the emitFactory (here) when run in MSIE11.

This is really unfortunate as I need to have support for internet explorer 11, and this makes select fields useless in that environment.

I am polyfilling custom events, so I'm pretty confident that something is wrong with the referenced line.

Would really appreciate some help on this! 🙏

@jamesmfriedman
Copy link
Collaborator

@astrofrans I'll be upgrading this library to the newest version of material-components-web in the near future. These maintenance upgrades are usually not that bad, except in the last 2 versions they have decided to continuously refactor the select. Not only is this maddening for me, it also causes more issues such as this. I can definitely look into the issue, but not before I have to re-rewrite the select again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants