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

Do not overwrite standard File API #316

Open
mark-veenstra opened this issue May 20, 2019 · 22 comments
Open

Do not overwrite standard File API #316

mark-veenstra opened this issue May 20, 2019 · 22 comments
Milestone

Comments

@mark-veenstra
Copy link

mark-veenstra commented May 20, 2019

Feature Request

Motivation Behind Feature

This plugin overwrites standard Web API implementations like:

Plugin Clobber Standard Web API
window.File File
window.FileReader FileReader
window.ProgressEvent ProgressEvent

This causes issues for example with the E2E testing tool Cypress. Specifically the Cypress plugin cypress-file-upload, which relies on the default File API implementation.

Feature Description

Do not overwrite the standard API. It seems that the plugin itself using it's own require method to include the correct JS file. But the plugin.xml file also overwrites the standard Web implementation.

Drawbacks would be that people that are using these exposed overwritten API's would have a problem or need to re-write code. That being said, the adjusted File API that this plugin needs could still be exposed on the window object, but not overwrite the standard API, for example in an own namespace or something like that.

For example expose:

  • window.File within a cordova namespace, like window.cordova.File

Alternatives or Workarounds

Alternatives would be that tools like Cypress or any other codebase that needs the standard API include a polyfill to get the standard API back again.

@janpio janpio added the support label Jun 7, 2019
@janpio
Copy link
Member

janpio commented Jun 7, 2019

Cordova Plugin File was implemented before (most of) the browsers had the specification implemented, which is why it uses the same namespace. Changing this now would be a major breaking change.

@janpio janpio added enhancement and removed support labels Jun 7, 2019
@JetUni
Copy link

JetUni commented Jun 18, 2019

We desperately need this to be fixed as well! It was a pain trying to debug why the File wasn't working as I expected it should have. Is there at least some way to get around this for the time being?

@janpio
Copy link
Member

janpio commented Jun 18, 2019

Don't use this plugin 🤷‍♀

I can't really think of a way to easily get rid of this baggage. Do you have any suggestions?


One could probably fork this plugin and change all the clobbers tags in plugin.xml and see if it magically works, but I kind of expect loads of cross dependencies between objects. Then those would have to be updated as well, and see if it works then. unfortunately test coverage of the plugin is not very good and brittle, so you might not catch everything that breaks by using that.

@JetUni
Copy link

JetUni commented Jun 18, 2019

The problem is that it's a dependency of cordova-plugin-ionic which we just started using. I don't know that there's any easy way to get rid of this baggage either, but having known that browsers were working on implementing this, a better option might have been as @mark-veenstra mentioned and use window.cordova.File. Of course, these kinds of changes may require a major version bump.

@mark-veenstra
Copy link
Author

I think it is not a question if it is easy or not, but more if it is needed or not.

The problem lies ofcourse that everyone can overwrite and pollute the window object. And this plugin realized a great solution in the past when the window.File and window.FileReader and window.ProgressEvent didn't exist yet.

But now they do other tooling will make use of this standard API, therefore it would be better to bump to a new major version and not overwrite the standard API.

@janpio
Copy link
Member

janpio commented Jun 19, 2019

I don't disagree with you both, I just know that there is nobody from the Apache Cordova team (all volunteers doing this in their free time) that needs this right now and more importantly has the time to start advocating for this and implementing it.

The plugin is the way it is because ~10 years ago this was a good decision - and now all kinds of things depend on it. And knowing how Javascript dependencies were handled over these 10 years, I am 100% sure that some of the tools using it will break because they automatically accept whatever new version we release. (Note: npm is only 9 years old)

(Another big factor is that the actual tests for this plugin currently are failing already, see #314. Current master is only green because the last release was 1.5 years ago and tests haven't been run for 3 months)

If there was a Pull Request doing the actual work and proving what exactly has to be changed, it would probably be a lot easier for Apache Cordova to "roll with it" and put in the additional required work of getting this to a release. (Which basically tells you: Do some of the work, then I will try to support you as well as I can.)

@benny-medflyt
Copy link

This looks like a duplicate of #265

@cosminadrianpopescu
Copy link

But at least you could just not completely overwrite the old API, like Zone is doing. So, before the var File = function(...), you could do something like this:

var OriginalFileApi = File;
var File = function(...){}

This would at least give us the possibility to just change some small portions of our code. I also depend on the cordova native HTTP plugin, which brings this one as dependency.

@cosminadrianpopescu
Copy link

I've created the PR #409 to achieve this.

@julia-fix
Copy link

Is there any progress on this issue? Or any way to get original FileReader while still using this plugin? Some of others plugin I use depends on this plugin so I cannot stop using it, but now I cannot add a new functionality to my app, such as recording media.
When I faced issue using media-capture plugin as it does not work with Android 10+, I thought new html5 and js features will give me an alternative, but no luck :(

@chr15m
Copy link

chr15m commented Dec 30, 2021

@July- what you can do is shim in some code before you load cordova.js in your index.html to save the old File etc.

I was very surprised when this plugin overwrote window.File and broke my app.

@breautek
Copy link
Contributor

I think the first step is to introduce a new access point for the Cordova API and deprecate the original one. We won't be able to remove the original one until a new major version, but we can start providing warnings for developers to migrate their code.

This way if the user wants to override the browser implementation, they have the flexibility of doing so themselves, depending on their use cases.

@breautek breautek added this to the 8.x milestone May 20, 2022
@phatpaul
Copy link

phatpaul commented Jul 1, 2022

@chr15m This is the way. Thanks for the hint.

FYI what I did in my index.html:

  <script>
    window.File_native = window.File; // backup the native File api because cordova-plugin-file clobbers it.
  </script>
  <script type="text/javascript" src="cordova.js"></script>
...

And when I needed to use the native File API in js:

                        let File = window.File;
                        if ("File_native" in window) { // cordova-plugin-file clobbers File(), so we backed it up at the beginning of index.html before loading cordova.js
                            // @ts-ignore
                            File = window["File_native"];
                        }
                        let otaBinary = new File([theBlob], fname);

@StarHosea
Copy link

@chr15m This is the way. Thanks for the hint.

FYI what I did in my index.html:

  <script>
    window.File_native = window.File; // backup the native File api because cordova-plugin-file clobbers it.
  </script>
  <script type="text/javascript" src="cordova.js"></script>
...

And when I needed to use the native File API in js:

                        let File = window.File;
                        if ("File_native" in window) { // cordova-plugin-file clobbers File(), so we backed it up at the beginning of index.html before loading cordova.js
                            // @ts-ignore
                            File = window["File_native"];
                        }
                        let otaBinary = new File([theBlob], fname);

Tried like this, sometimes work fine, sometimes not , i didn't know why

@kfeng0806
Copy link

kfeng0806 commented Feb 1, 2023

A temporary fix if there is no other plugin depending on the plugin's File & FileReader class

const nativeFile = window.File;
const nativeFileReader = window.FileReader;

window.document.addEventListener('deviceready', () => {
    window.CordovaFile = window.File;
    window.CordovaFileReader = window.FileReader;

    window.File = nativeFile;
    window.FileReader = nativeFileReader;
});

Non-conflict on the browser's native File and FileReader, CordovaFile will be the plugin's File class

@LucasBourgeois
Copy link

Please fix this

@chr15m
Copy link

chr15m commented Jun 27, 2023

Dear @LucasBourgeois, if you want it fixed then put a fix together and submit a pull request.

@LucasBourgeois
Copy link

There is already one waiting to be merged.
#409

It, at least, allow to have native api's available.

Fearing breaking changes because of legacy implementation and not releasing a major version update is really a shame!

If something breaks in ppl repos, it'll be for the best as this bug is causing more bugs that not changing this is causing...

@jacobg
Copy link

jacobg commented Jan 22, 2024

Changing the standard API's is a really bad idea. Is there a fork that fixes this, or is there another plugin that accomplishes the same thing without changing the standard API?

@qiutian00
Copy link

Changing the standard API's is a really bad idea, It effects other upload file components that use window.File

@qiutian00
Copy link

qiutian00 commented Jun 16, 2024

In plugin.xml or cordova-plugin.js comment <clobbers target="window.File" />, not overwrite window.File, and restart the app

<js-module src="www/File.js" name="File">
    <!--    <clobbers target="window.File" /> -->
</js-module>

@breautek
Copy link
Contributor

In plugin.xml or cordova-plugin.js comment <clobbers target="window.File" />, not overwrite window.File, and restart the app

<js-module src="www/File.js" name="File">
    <!--    <clobbers target="window.File" /> -->
</js-module>

This is effectively disabling the File plugin's JS code. If you're going to do this, you should consider if the file plugin is even necessary/being used and remove it if it's not. If your application actually uses the file plugin or you use any plugins that depend on the file plugin API, you'll be breaking those parts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests