fix: onchange event with preact/compat enabled #72
Conversation
this fix aligns this library with testing-library for React where fireEvent.change() works as expected
onChange test requires import from preact/compat which affects other tests in the same test file so I moved it to separate file
src/fire-event.js
Outdated
| // Preact changes all change events to input events. This is normally handled directly | ||
| // but when running 'preact/compat' this is done via custom JS which renames the event prop, | ||
| // making the event name out of sync. | ||
| // The problematic code is in: preact/compat/src/render.js > handleDomVNode() | ||
| const keyFiltered = key === 'change' ? 'input' : key |
There was a problem hiding this comment.
Preact only aliases change events to input events when preact/compat is used, to match React's behavior.
Aliasing as you have here, unconditionally, breaks functionality for anyone not using preact/compat, and therefore expecting the native change event. Need some way to apply this only when preact/compat is in use.
There was a problem hiding this comment.
You are absolutely correct, I misunderstood this in their documentation. I will look for a possible way to do this and update my pull request. Thanks for the review!
There was a problem hiding this comment.
I am thinking to apply the check if compat is used by testing if one (or more) of the functions/objects exported from it exist. My main candidates are:
forwardRef: but it might be discontinued as hinted here The road to Preact 11 preactjs/preact#2621__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: I like that the name is pretty much guaranteed to be unique to this library, but at the same time might eventually be removed from React.memo: may not be unique enough.
Please what do you think? Do you have any suggestions? Thank you!
There was a problem hiding this comment.
import { options } from 'preact';
let isCompat = false;
const oldHook = options.vnode;
options.vnode = vnode => {
if (vnode['$$typeof']) isCompat = true;
if (oldHook) oldHook(vnode);
}This will detect preact/compat usage.
There was a problem hiding this comment.
Beautiful, thank you very much! Latest commit should now contain all the requested changes. :)
replace unnecessary forwardRef import with general preact/compat; improve commentary Co-authored-by: Ryan Christian <33403762+rschristian@users.noreply.github.com>
prevent applying aliasing when compat library is not used; add change event to basic event tests to ensure both cases work
|
@JoviDeCroock, sorry, I don't have permissions here. |
|
Seems like the tests are failing 😅 |
|
Optional chaining in a dep by the looks of it. Shouldn't really matter here, Node 12 is pretty irrelevant these days. I'll make a PR in a minute to bump them. |
JoviDeCroock
changed the title
What:
This is a fix to a bug that I found: vitest-dev/vitest#5456
It took me a while to figure out where exactly is the issue so the communication spans vitest and preact forums as well.
Why:
Because
preact/compatlibrary renames props before establishing events, the events are listening under different name (specifically,onChangelistens toinputevent). So, when firing event viafireEvent.change()the event also needs to be translated so that it gets triggered.This can be avoided in code by using
fireEvent.input()in tests, but since Preact documentation does not imperatively rule out usingonChangeI think the testing library should support it also. Furthermore,onChangeis available because of compatibility with React wherefireEvent.change()also works.How:
Added condition into fire-event.js.
Added dedicated test for this condition.
Both are properly commented to explain the logic and reasoning.
Checklist:
Comments:
During testing I also found out there are 15 other events which have issues because of this (mostly animation, touch and composition events, and also focus and blur). If you agree with my approach, I could also try to fix the rest in a separate branch.