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

Add UT to reproduces #106 #108

Closed
wants to merge 1 commit into from

Conversation

buenjybar
Copy link

What kind of change does this PR introduce?

  • Bug Fix
  • Feature
  • Refactoring
  • Style
  • Build
  • Chore
  • Documentation
  • CI

Did you add tests for your changes?

  • Yes, my code is well tested
  • Not relevant

If relevant, did you update the documentation?

  • Yes, I've updated the documentation
  • Not relevant

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 (#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
Copy link
Collaborator

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

@jonkoops
Copy link
Collaborator

jonkoops commented Nov 9, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

jonkoops commented Nov 9, 2024

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
Copy link
Collaborator

@buenjybar with #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
Copy link
Author

buenjybar commented Nov 13, 2024

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
Copy link
Collaborator

@buenjybar are you triggering a drop event?

@rolandjitsu
Copy link
Collaborator

I've also just tried it in react-dropzone-nextjs and I can also see the error. I'll try the patch in #107 and see if that works.

@rolandjitsu
Copy link
Collaborator

@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
Copy link
Collaborator

cypress-io/cypress#669 may also be helpful.

@buenjybar
Copy link
Author

Obsolete, It has been fixed in 2.1.2

@buenjybar buenjybar closed this Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants