Copy non-enumerable data transfer props

[Snizhana]

What:

Change the logic of copying dataTransfer props

Why:

At the moment Object.assign is used to extend event object with passed data, but doesn't copy non-enumerable props

How:

In order to iterate by non-enumerable props getOwnPropertyNames method is used

Checklist:

• Documentation added to the
  docs site
• Tests
• Typescript definitions updated
• Ready to be merged
  

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ea9a4bf:

Sandbox	Source
kentcdodds/react-testing-library-examples	Configuration

[codecov]

Codecov Report

Merging #734 into master will not change coverage.
The diff coverage is n/a.

@@ Coverage Diff @@ ## master #734 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 24 24 Lines 657 657 Branches 173 173 ========================================= Hits 657 657 

Impacted Files	Coverage Δ
src/events.js	100.00% <ø> (ø)

---

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 01e0242...ea9a4bf. Read the comment docs.

[kentcdodds]

Looks super! Thanks for this. For this one, a good test would be something that does what can't be done without these changes.

[Snizhana]

Looks super! Thanks for this. For this one, a good test would be something that does what can't be done without these changes.

thank you, I'll add tests. one more thing is there a guide on manual verification?

[eps1lon]

thank you, I'll add tests. one more thing is there a guide on manual verification?

Every PR is build in codesandbox. In the status list you can find ci/codesandbox. You could create a new codesandbox and then add @testing-library/dom but replace the version with the most recent build (https://pkg.csb.dev/testing-library/dom-testing-library/commit/162d81bc/@testing-library/dom) as of this writing.

That way you can share your manual test. Otherwise follow the "local install instructions".

[Snizhana]

thank you, I'll add tests. one more thing is there a guide on manual verification?

Every PR is build in codesandbox. In the status list you can find ci/codesandbox. You could create a new codesandbox and then add @testing-library/dom but replace the version with the most recent build (https://pkg.csb.dev/testing-library/dom-testing-library/commit/162d81bc/@testing-library/dom) as of this writing.

That way you can share your manual test. Otherwise follow the "local install instructions".

thanks, you helped me a lot

[kentcdodds]

Thanks for working on this. The test seems to be testing something very specific. Is there a typical use case of a test that a developer would write that doesn't work without your changes?

[Snizhana]

Thanks for working on this. The test seems to be testing something very specific. Is there a typical use case of a test that a developer would write that doesn't work without your changes?

I have tests similar to this for a while on my project. But they started to fail when I upgraded react testing library from 9.2.0 to 10.2.1 (onFileUpload is called now with an empty array, because it is overwritten here). This issue effects users who run tests in the browser env and try to mock any non-enumerable dataTransfer props. as a workaround I started to remove DataTransfer constructor from the window before triggering drop event in test to not execute the line

function FileUploader ({ onFileUpload }) { function handleDrop(e) { onFileUpload(e.dataTransfer.items); } return <div onDrop={handleDrop} /> } it('should upload file', () => { const onFileUploadSpy = jasmine.createSpy('onFileUploadSpy'); const component = render(<FileUploader onFileUpload={onFileUploadSpy}/>); const dragNdropZone = component.getByTestId('drag-n-drop-zone'); const dataTransfer = new DataTransfer(); Object.defineProperty(dataTransfer, 'items', {value: [{ name: 'test-file' }]}); expect(onFileUploadSpy).toHaveBeenCalledWith(dataTransfer.items); })

[eps1lon]

Looks like the test is mocking how DataTransfer would look in a browser. Though items wouldn't be a property on the object but the prototype.

Did your test work in a browser or only in JSDOM?

[Snizhana]

Looks like the test is mocking how DataTransfer would look in a browser. Though items wouldn't be a property on the object but the prototype.

Did your test work in a browser or only in JSDOM?

Basically DataTransfer mocked implementation is not important here. In order to test changes we need to make sure that it is available in window as a function. DataTransfer instance prop items is not enumerable, so it wasn't copied by Object.assign

I tested changes in karma env

[eps1lon]

I tested changes in karma env

With your mock or in a browser that implements DataTransfer? I tried it in chrome 84 devtools and Object.getOwnPropertyNames(new DataTransfer()) is empty because items isn't a property on the object by spec.

It seems to me that

const dt = new DataTransfer(); const file = new File([], 'empty.png'); dt.items.add(file)

would work in a browser. To get the correct functionality into an environment without a standard DataTransfer the following implementation would be closer (though still incomplete):

class File { constructor(fileBits, fileName, options = {}) { this._name = fileName; this._lastModified = options._lastModified || 0; } get name() { return this._name } get lastModified() { return this._lastModified } } class DataTransferItem { constructor(file) { // private this._file = file; } get kind() { return "file" } get type() { throw new Error('not implemented') } getAsString() { throw new Error('not implemented') } getAsFile() { return this._file } } class DataTransferItemList { constructor() { this._items = [] } // only implements DataTransferItem? add(File data); add(data) { const item = new DataTransferItem(data); this._items.push(item); return item; } } class DataTransfer { constructor() { this._items = new DataTransferItemList(); } get items() { return this._items } }

[Snizhana]

I tested changes in karma env

With your mock or in a browser that implements DataTransfer? I tried it in chrome 84 devtools and Object.getOwnPropertyNames(new DataTransfer()) is empty because items isn't a property on the object by spec.

It seems to me that

const dt = new DataTransfer(); const file = new File([], 'empty.png'); dt.items.add(file)

would work in a browser. To get the correct functionality into an environment without a standard DataTransfer the following implementation would be closer (though still incomplete):

class File { constructor(fileBits, fileName, options = {}) { this._name = fileName; this._lastModified = options._lastModified || 0; } get name() { return this._name } get lastModified() { return this._lastModified } } class DataTransferItem { constructor(file) { // private this._file = file; } get kind() { return "file" } get type() { throw new Error('not implemented') } getAsString() { throw new Error('not implemented') } getAsFile() { return this._file } } class DataTransferItemList { constructor() { this._items = [] } // only implements DataTransferItem? add(File data); add(data) { const item = new DataTransferItem(data); this._items.push(item); return item; } } class DataTransfer { constructor() { this._items = new DataTransferItemList(); } get items() { return this._items } }

unfortunately mocked DataTransfer implementation doesn't work. I got the error. I tried mock you posted and an example from docs

TypeError: Failed to construct ‘DragEvent’: member dataTransfer is not of type DataTransfer.

[micha149]

I think this could just be an empty object to reduce the complexity and focus on whats relevant for this testcase

[Snizhana]

good point

[micha149]

TypeError: Failed to construct ‘DragEvent’: member dataTransfer is not of type DataTransfer.

This is another problem in createEvent. The eventInit object is just passed to the event constructor and the DataTransfer instance will be created on the newly created event instance. Problem here is that the event constructor fails, because dataTransfer isn't the correct instance at this moment.

The conversion mechanism need to be placed before construction the event instance. But I think this is another bug and could be done in a second PR.

[Snizhana]

TypeError: Failed to construct ‘DragEvent’: member dataTransfer is not of type DataTransfer.

This is another problem in createEvent. The eventInit object is just passed to the event constructor and the DataTransfer instance will be created on the newly created event instance. Problem here is that the event constructor fails, because dataTransfer isn't the correct instance at this moment.

The conversion mechanism need to be placed before construction the event instance. But I think this is another bug and could be done in a second PR.

yeah, I agree this is a different issue, my point is that I am not able to use a suggested solution on my test

[eps1lon]

unfortunately mocked DataTransfer implementation doesn't work. I got the error. I tried mock you posted and an example from docs

Yeah in a browser with a real DataTransfer you can't mock it.

[micha149]

Ah, ok. I can't too. We just came across this exact problem. Our test looks like:

it('shows overlay if file is dragged over drop zone', async () => { uploadType = { singular: 'Image', plural: 'Images', filter: ['image/png'], }; await setup({uploadType}); const fileZone = screen.getByTestId('drop-zone'); const dataTransfer = new DataTransfer(); Object.defineProperty(dataTransfer, 'types', { value: ['Files']}) fireEvent.dragEnter(fileZone, { dataTransfer }); const fileOverlay = screen.getByText('Upload images...'); expect(fileOverlay).toBeTruthy(); });

After some debugging I came to mostly your solution. With your changes our tests work as expected.

[micha149]

Can anyone tell why the tests are failing? I don't see any relations to the changes made.

[Snizhana]

Can anyone tell why the tests are failing? I don't see any relations to the changes made.

I had the same issue locally after pulling the latest changes. I reinstalled node modules and the issue has gone. how can I reinstall dependencies for the build?

[kentcdodds]

I just cleared the cache and restarted the build 👍

[kentcdodds]

Ok, so I'm thinking that because JSDOM doesn't support the DataTransfer constructor, this is probably the best we can do for a test and I'm ok with that. Anyone else have thoughts before we merge this as-is?

[micha149]

Wherever my prior approval has gone, I'm still fine with this change

[micha149]

At least 1 approving review is required by reviewers with write access.

🤦 Now I noticed the „with write access”. Never answer in issues before your first coffee. ☕

Sorry for that.

[kentcdodds]

Thanks a bunch!

[kentcdodds]

@all-contributors please add @Snizhana for code and tests

[allcontributors]

@kentcdodds

I've put up a pull request to add @Snizhana! 🎉

[kentcdodds]

@all-contributors please add @micha149 for review

[allcontributors]

@kentcdodds

I've put up a pull request to add @micha149! 🎉

[kentcdodds]

@Snizhana, thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

[micha149]

Is someone already on a fix for the costructor error? If not I would give it a try, because it will make our tests a bit more readable…

[kentcdodds]

🎉 This PR is included in version 7.22.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀