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

[Feature] Support File API polyfill #71

Closed
mark-veenstra opened this issue May 16, 2019 · 20 comments
Closed

[Feature] Support File API polyfill #71

mark-veenstra opened this issue May 16, 2019 · 20 comments
Assignees
Labels
enhancement New feature or request polyfill wontfix This will not be worked on

Comments

@mark-veenstra
Copy link

mark-veenstra commented May 16, 2019

Current behavior:

It throws the following error:

TypeError: Failed to execute 'add' on 'DataTransferItemList': parameter 1 is not of type 'File'.

Desired behavior:

That the PDF file is uploaded

Steps to reproduce: (app code and test code)

I try to upload the PDF with the following different solutions of the code. Using force: true because elements are hidden:

TEST 1:

    cy.fixture('mypdffile-from-fixtures.pdf', 'base64').then(fileContent => {
        cy.get(`body [data-test="file-upload"][data-cy="mypdffile-from-fixtures.pdf"]`).upload(
            {fileContent, fileName: 'mypdffile-from-fixtures.pdf', mimeType: 'application/pdf'},
            {subjectType: 'input', force: true}
        );
    });

TEST 2:

    cy.fixture('mypdffile-from-fixtures.pdf', 'base64').then(fileContent => {
        cy.get(`body [data-test="file-upload"][data-cy="mypdffile-from-fixtures.pdf"]`).upload(
            {fileContent, fileName: 'mypdffile-from-fixtures.pdf', mimeType: 'application/pdf', encoding: 'base64'},
            {subjectType: 'input', force: true}
        );
    });

TEST 3:

    cy.fixture('mypdffile-from-fixtures.pdf', 'utf8').then(fileContent => {
        cy.get(`body [data-test="file-upload"][data-cy="mypdffile-from-fixtures.pdf"]`).upload(
            {fileContent, fileName: 'mypdffile-from-fixtures.pdf', mimeType: 'application/pdf', encoding: 'utf8'},
            {subjectType: 'input', force: true}
        );
    });

TEST 4:

    cy.fixture('mypdffile-from-fixtures.pdf').then(fileContent => {
        cy.get(`body [data-test="file-upload"][data-cy="mypdffile-from-fixtures.pdf"]`).upload(
            {fileContent, fileName: 'mypdffile-from-fixtures.pdf', mimeType: 'application/pdf'},
            {subjectType: 'input', force: true}
        );
    });

I run Cypress in a docker image and the image does see the file:

mva@mva-laptop:~/Documents/Projects/client$ docker run --rm --name=e2e_test --ipc=host -ti -v $(pwd)/config:/usr/src/app/config -v $(pwd)/cypress:/usr/src/app/cypress ${DOCKER_IMAGE} sh -c "ls -la cypress/fixtures/"
total 80
drwxr-xr-x 2 node node  4096 May 16 11:08 .
drwxrwxr-x 9 node node  4096 May 10 11:22 ..
-rw-r--r-- 1 node node 70969 May 16 11:08 mypdffile-from-fixtures.pdf

Versions

Cypress 3.2.0

@mark-veenstra mark-veenstra added the bug Something isn't working label May 16, 2019
@abramenal
Copy link
Owner

Hi @mark-veenstra
Thanks for submitting the issue. Let's have a look on it together starting with easy questions.
Can you please share your markup for the layout?
Have you tried to log fileContent value before trying to upload it?

@mark-veenstra
Copy link
Author

mark-veenstra commented May 20, 2019

@abramenal Thnx for helping out.

I used TEST 4 from above (no encoding at all).

If I open the console within Cypress, the fileContent has some content, see next screenshot:
image

@mark-veenstra
Copy link
Author

@abramenal I also added a breakpoint on this line where the error is thrown. At that point e which is this line with the var file is also filled as follows:
image

@abramenal
Copy link
Owner

@mark-veenstra I've removed the original file content from your comment as it blows the comments history 😄
Okay, got this, but could you please also share the HTML content of your view? Is it plain HTML/JS or any framework-specific component? I would love seeing it as it may also answer the question you're asking.

@mark-veenstra
Copy link
Author

mark-veenstra commented May 20, 2019

@abramenal We are using cordova and it seems that the plugin cordova-plugin-file is overwriting the standard window.File() implementation. As shown here.

When I remove this specific cordova implementation of window.File() and I use the bsase64 enconding all works fine.

    cy.fixture('mypdffile-from-fixtures.pdf', 'base64').then(fileContent => {
        cy.get(`body [data-test="file-upload"][data-cy="mypdffile-from-fixtures.pdf"]`).upload(
            {fileContent, fileName: 'mypdffile-from-fixtures.pdf', mimeType: 'application/pdf', encoding: 'base64'},
            {subjectType: 'input', force: true}
        );
    });

Now I have to figure out how I can use this cordova plugin and also use your plugin to automate some testing.

@abramenal
Copy link
Owner

abramenal commented May 20, 2019

@mark-veenstra that's actually a great finding!
We may think on encapsulating/forced polyfilling of the dependencies we use internally (exactly like window.File).
Example: https://github.com/mailru/FileAPI (maybe not really mail.ru implementation 😈 but still same idea)

Will investigate that thing this week. Help appreciated.

@mark-veenstra
Copy link
Author

That repository is mentioned on the modernizr Wiki: https://github.com/Modernizr/Modernizr/wiki/HTML5-Cross-Browser-Polyfills#file-api

@mark-veenstra
Copy link
Author

@abramenal Did you close this issue for a reason?

I did open an issue on the issue tracker of Cordova, but as you can see they will not do this within a short notice since it is a breaking change. It can even be that this takes years for them to solve. I have posted more issue on that tracker and some take months to years to solve.

Could we still add a polyfill in this repository? Or do you want to wait on Cordova to make some changes?

@abramenal
Copy link
Owner

@mark-veenstra
I saw the issue you open against Cordova and feel this is the only right way of doing that.
But if you say it can take years that's not an option then 😄
Unfortunately at the moment I cannot invest much time on this specific issue, especially since I feel you're the only one who reported on this. Anyway I'm open to design and code that properly as it might come as an issue for further platforms, users, etc in future.

Ideally it can be a separate method that a user can import (like initialize) where you can put usePolyfill property set to true and that's it. What do you think?
In the mean time if you have some time to start coding this I would be really happy! That'll significantly speed up releasing of these changes.

@abramenal abramenal reopened this Jun 17, 2019
@abramenal abramenal added polyfill help wanted Extra attention is needed labels Jun 17, 2019
@mark-veenstra
Copy link
Author

@abramenal I could also fork the cordova plugin and expose the window.File etc to a differrent namespace and include that forked and adjusted plugin in my project. I understand that you have lack of time and that Cordova should fix this.

Adjusting and forking the plugin is quite easy and a quick win on my side.

@abramenal
Copy link
Owner

@mark-veenstra
Great! Then please investigate that one, and if it's still easier to adjust that in this plugin, let's think further 😄
Please let me know when you finally decide.

@abramenal
Copy link
Owner

@mark-veenstra any updates on that?

@mark-veenstra
Copy link
Author

I created a fork and changed the way the plugin exposes to the window object and all seems to work.

@abramenal
Copy link
Owner

@mark-veenstra great!
It seems our issue can now be closed, am I right?

@mark-veenstra
Copy link
Author

Well as long as cordova-plugin-file does not solve it and people use this plugin together with your cypress plugin it will fail. People could try the same "hack" I did. Or hopefully a major release in the future of cordova-plugin-file will still solve it completely.

Or as we said before a shim is needed in this plugin. But that feels a bit strange because it a standard API you are using.

@abramenal
Copy link
Owner

@mark-veenstra
I understand the pain and see there's no chance apache team will take that.

I don't see any harm in making the change in this plugin, especially if it can be made safely.

If you have time, could you please try experimenting with the plugin?
The line needed is: https://github.com/abramenal/cypress-file-upload/blob/master/src/upload.js#L22

What I would be okay with is having an extra config field called usePolyfills, and if you pass that, you can decide how to create a file – using native endpoint or a polyfill (that should be a part of separate helper function).

Here is a list of polyfills to start with:
https://github.com/Modernizr/Modernizr/wiki/HTML5-Cross-Browser-Polyfills#file-api

@abramenal
Copy link
Owner

@mark-veenstra
Did you have a chance to take a look? If not, that's fine, I may start doing this as well now 😃

@mark-veenstra
Copy link
Author

Unfortunately not

@abramenal abramenal added enhancement New feature or request and removed bug Something isn't working labels Jul 13, 2019
@abramenal abramenal changed the title [Bug] Fails to upload pdf [Feature] Support File API polyfill Jul 13, 2019
@abramenal
Copy link
Owner

Hey @mark-veenstra
Were you able to progress any of the changes discussed? I wasn't able to take a look.
I feel this feature obsolete as for now as no other users were requesting this as well. Feel free to open a PR with these changes if you still feel them reasonable so we could decide. Right now I will close the issue as "won't do" thing (at least for now). Thank you for understanding.

@abramenal abramenal added wontfix This will not be worked on and removed help wanted Extra attention is needed labels Sep 21, 2019
@mark-veenstra
Copy link
Author

We are working with a fork of the Cordova plugin for now and changed the expose to a Cordova namespaces window object. Which solves all issues (for now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request polyfill wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants