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

Production use question #1

Closed
chrisblossom opened this issue May 22, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@chrisblossom
Copy link

commented May 22, 2017

I ran into this looking for a way to manually trigger an onClick event. Why do you recommend against production use?

Here is the code I am trying to replace:
https://github.com/chrisblossom/react-safe-universal-inputs/blob/master/src/InputWrapper.js#L25

Would appreciate any suggestions.

Thank you!

@vitalyq

This comment has been minimized.

Copy link
Owner

commented May 23, 2017

Hey @chrisblossom,

  • It is a bad practice to depend on library's implementation details or private fields. Violating it leads to maintenance pain. Even a minor patch in React can break this library.

  • Triggering events produces some side effects. For example, calling reactTriggerChange on text input will trigger focus and input synthetic events in addition to change synthetic event. Side effects are different for various element types. It is OK for end-to-end testing because most of these events are triggered by the user input anyway, but may be not desired when reactTriggerChange is called by the application.

  • Dispatched events are different from native events. For example, event.isTrusted on triggered events is always false, event.nativeEvent contains events created with createEvent.

  • It may be tempting to trigger change event on a component's element from another component:

    const handleClick = (event) => {
      event.preventDefault();
      const node = document.getElementsByTagName('input')[0];
      reactTriggerChange(node);
    };
    
    ReactDOM.render(
      <form>
        <input onChange={() => console.log('changed')} />
        <button onClick={handleClick}>Trigger change</button>
      </form>,
      document.getElementById('root'),
    );

    The dependency between input and button is hard to reason about. This is against the React's "unidirectional" data flow model and should be avoided.

In my vision, your input wrappers could provide a separate API to access early input, let's say onEarlyInput property. If early input is present, the onEarlyInput function is called with a single argument (early input value).

Another solution is to skip providing event properties other than target.value or implement an API to provide additional properties. I think it is reasonable, considering ReactTestUtils.Simulate doesn't provide event properties by default as well.

@chrisblossom

This comment has been minimized.

Copy link
Author

commented May 25, 2017

Thank you for the well thought out reply.

I think moving to your suggestion using onEarlyInput is the best path forward to minimize unexpected behavior with other events such as focus.

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