-
Notifications
You must be signed in to change notification settings - Fork 113
Add support for input file on fireEvent.update
#179
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
Add support for input file on fireEvent.update
#179
Conversation
Hi! Can't fix it right now, but you can use userEvent.upload(). It works fine with your example.
That being said, I think the problem is that we should avoid setting value attribute when type is file (source):
case 'INPUT': { if (['checkbox', 'radio'].includes(type)) { elem.checked = true return fireEvent.change(elem) } else if (type === 'file') { return fireEvent.input(elem) } else { elem.value = value return fireEvent.input(elem) } }
With that, the following test passes (copied from the test you linked in the issue):
test('fireEvent.update should not crash with input file', async () => { const {getByTestId} = render({ render(h) { return h('input', { attrs: { type: 'file', 'data-testid': 'test-update', }, }) }, }) const file = new File(['(⌐□しろいしかく_□しろいしかく)'], 'chucknorris.png', { type: 'image/png', }) const inputEl = getByTestId('test-update') Object.defineProperty(inputEl, 'files', { value: [file], }) await fireEvent.update(inputEl) expect(console.warn).not.toHaveBeenCalled() })
if (['checkbox', 'radio'].includes(type)) { elem.checked = true return fireEvent.change(elem) } else if (type === 'file') { Object.defineProperty(elem, 'files', { value }) return fireEvent.change(elem) } else { elem.value = value return fireEvent.input(elem) }
Why can't it be like this? instead of doing manual population of files property every time we use input file?
Yeah, it works fine with upload and works just fine even with using just fireEvent.change manually, the only reason I want it by default is to avoid the warning that's all.
if (['checkbox', 'radio'].includes(type)) { elem.checked = true return fireEvent.change(elem) } else if (type === 'file') { Object.defineProperty(elem, 'files', { value }) return fireEvent.change(elem) } else { elem.value = value return fireEvent.input(elem) }Why can't it be like this? instead of doing manual population of files property every time we use input file?
That would make sense. However I'm not sure if we should step into userEvent realm with fireEvent. The only reason fireEvent.update exists is to overcome some limitations when it comes to async DOM updates. Now, handling <input type="file"> differenty is probably unexpected, and better handled by userEvent.
I'm ok with adding the else if case to avoid the warning when adding files, though 👍
Yeah, you're right with the purpose of fireEvent.update. It should only be used when we're using the v-model or value/ (change/input) case and for the case of the input file, we're not allowed to set up a value so the ability to turn off the warning is fine.
I'd definitely add the else if, and also a test example on how to input a file! Fancy to do so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this test example not enough? @afontcu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is! But as mentioned, I wouldn't make fireEvent.update() smarter when it comes to type="file" inputs. I would just trigger the proper event, and then add a test showcasing how to handle files as in React Testing Library (and probably also a comment suggesting userEvent to do so:
test('fireEvent.update should not crash with input file', async () => { const {getByTestId} = render({ template: `<input type="file" data-testid="test-update" />` }) const file = new File(['(⌐□しろいしかく_□しろいしかく)'], 'chucknorris.png', { type: 'image/png' }) const inputEl = getByTestId('test-update') Object.defineProperty(inputEl, 'files', { value: [file], }) await fireEvent.update(inputEl) expect(console.warn).not.toHaveBeenCalled() })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a suggestion with the change I mention above 👍
Codecov Report
@@ Coverage Diff @@ ## master #179 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 1 1 Lines 98 100 +2 Branches 34 35 +1 ========================================= + Hits 98 100 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also not 100% sure if we should trigger a change event over an input one? 🤔 haven't checked, so it might be okay)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems pretty okay to me since we know we can't use v-model to an input file and the only reasonable event to use in this case is change.
fireEvent.update (追記ここまで)
nice! 🙌
#177