react-dropzone / file-selector

Convert a DragEvent or file input to a list of File objects
MIT License
95 stars 33 forks source link

Add UT to reproduces #106 #108

Open buenjybar opened 1 week ago

buenjybar commented 1 week ago

What kind of change does this PR introduce?

Did you add tests for your changes?

If relevant, did you update the documentation?

Summary Explain the motivation for making this change. What existing problem does the pull request solve? Try to link to an open issue for more information.

Try to reproduce bug #106 (https://github.com/react-dropzone/file-selector/issues/106)

Does this PR introduce a breaking change? If this PR introduces a breaking change, please describe the impact and a migration path for existing applications.

Other information

rolandjitsu commented 1 week ago

Hmm ... I wonder why checks haven't run 😕

jonkoops commented 1 week ago

They need to be approved, which only maintainers can do. This is for security reasons so that people cannot change workflows in a way that would be considered insecure.

rolandjitsu commented 1 week ago

They need to be approved, which only maintainers can do. This is for security reasons so that people cannot change workflows in a way that would be considered insecure.

I think the new UI doesn't have that.

Screenshot 2024-11-09 at 20 56 23
jonkoops commented 1 week ago

Oh interesting, I am still on the old one so I do see it, of course I can't start it as I don't have the permission to do so.

rolandjitsu commented 1 week ago

@buenjybar with https://github.com/react-dropzone/file-selector/pull/107, you'll now get a promise rejection with:

"[object Object] is not a File"

See diff.

So you'll need to handle that. But what that test means, essentially, is that the object you get is not actually a file, see return value:

A Promise.

If the item's kind property is "file", and this item is accessed in the dragstart or drop event handlers, then the returned promise is fulfilled with a FileSystemFileHandle if the dragged item is a file or a FileSystemDirectoryHandle if the dragged item is a directory.

Otherwise, the promise fulfills with null.

And there's no way to reach that point with any other event than a drop because there's a guard for that:

        // According to https://html.spec.whatwg.org/multipage/dnd.html#dndevents,
        // only 'dragstart' and 'drop' has access to the data (source node)
        if (type !== 'drop') {
            return items;
        }
        const files = await Promise.all(items.map(toFilePromises));

If this happens during cypress tests, it might be worth seeing if there's something about that which may be causing these issues. Are you using something like https://www.cypress.io/blog/uploading-files-with-selectfile?

buenjybar commented 6 days ago

It seems that cypress is using the dispatchEvent with the following event type: dragenter

so it is never catch by this code :

        // According to https://html.spec.whatwg.org/multipage/dnd.html#dndevents,
        // only 'dragstart' and 'drop' has access to the data (source node)
        if (type !== 'drop') {
            return items;
        }
        const files = await Promise.all(items.map(toFilePromises));

I checked in this project the items variable just above look like this image

It is actually a valid File.

rolandjitsu commented 3 days ago

@buenjybar are you triggering a drop event?

rolandjitsu commented 3 days ago

I've also just tried it in react-dropzone-nextjs and I can also see the error. I'll try the patch in https://github.com/react-dropzone/file-selector/pull/107 and see if that works.

rolandjitsu commented 3 days ago

@buenjybar I also suggest opening a ticket with cypress and inform them of the current behaviour. We expect that the promise resolves to a file, but it looks like it doesn't.

We could add a check in our code for window.Cypress and then handle that case ... but I feel that they should tell us how to deal with that.

It might also be a permission thing or https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts ...

rolandjitsu commented 3 days ago

https://github.com/cypress-io/cypress/issues/669 may also be helpful.