search

apache / cordova-plugin-file

Apache Cordova File Plugin
https://cordova.apache.org/
Apache License 2.0
742 stars 761 forks source link

Promises #637

Open GitToTheHub opened 1 month ago

GitToTheHub commented 1 month ago

Feature Request

I want to add Promise functionality to this plugin and have some questions.

From my point, Promises are save to add natively, because:

Here a sample of a conversion i would make.

Old code:

FileEntry.prototype.file = function (successCallback, errorCallback) {
    const localURL = this.toInternalURL();
    const win = successCallback && function (f) {
        const file = new File(f.name, localURL, f.type, f.lastModifiedDate, f.size);
        successCallback(file);
    };
    const fail = errorCallback && function (code) {
        errorCallback(new FileError(code));
    };
    exec(win, fail, 'File', 'getFileMetadata', [localURL]);
};

New code:

FileEntry.prototype.file = function (successCallback, errorCallback) {
    return new Promise((resolve, reject) => {
        const localURL = this.toInternalURL();
        const win = function (f) {
            const file = new File(f.name, localURL, f.type, f.lastModifiedDate, f.size);
            if (successCallback) successCallback(file);
            resolve(file);
        };
        const fail = function (code) {
            if (errorCallback) errorCallback(new FileError(code));
            reject(new FileError(code));
        };
        exec(win, fail, 'File', 'getFileMetadata', [localURL]);
    });
};

Would it be fine like this?

Regards

breautek commented 1 month ago

The file plugin is written to W3C specs with the idea that eventually the plugin won't be needed eventually (in fact most Apache plugins were written like this).

They are as follows:

Given that the standard of this file plugin is based on is discontinued (with no known replacements as far as I know) I think we should be open to the idea of introducing Promise support where it makes sense. But we should not deviate from any active/working specifications, which means we should not make API changes to FileReader, FileWriter, Blob, etc... unless if the goal is to polyfill missing features.

It would also be worth seeing if Promises can be implemented in such a way that it isn't a breaking change, which I believe your sample proposal shows.

I suggest requesting a "buy-in" before starting any substantial work to see if other community members agree or disagree. (They don't always pay attention to GitHub notifications since they are quite noisy). To do this you'll need to subscribe to the Dev Mailing List and write to the list stating your intent. You an can reference this issue. If after 48-72 hours occurs with no response, then you can treat the lack of response as a lazy agreement.

For what it's worth I think introducing promise support on the filesystem APIs would cut down on a lot of "callback hell" that the W3C specification has and I personally see no problem deviating from a standard that appears to be killed. I believe the only browser that had an actual implementation of it was Chrome.

The devil's advocate here is that it's also easy to provide a Facade to provide the promise-based API without changing the underlying library API.

GitToTheHub commented 1 month ago

Hi @breautek,

thanks four thoughts and suggestions.

So you think it's ok to change something on the filesystem api, but not on the file api, except to make a Facade for it or for everything.

My intention was, to make it backward compatible. You can still use callbacks, but also promises, if no callbacks are given. What do you think how a facade could look like? It would be also ok, to change only the filesystem api to promises, this would, as you wrote, already cut down the callback hell.

I will do it like you wrote with the mailing list.

Regards

GitToTheHub commented 1 month ago

@breautek Nobody reacted on this except you, is this a lazy agreement?

breautek commented 1 month ago

@breautek Nobody reacted on this except you, is this a lazy agreement?

I would send out a "final call" email which can read something to the effect of:

Final call for comments before I start working on this feature. If there are no objections within then next 48 hours then I'll begin work as outlined.

If no one else responds, then yes I would treat it as a lazy agreement. Given this is just to decide if it's worth starting the work, the final call isn't really necessary (because after the work is done, a PR needs to be approved and merged still) BUT this serves as a final reminder for someone to make constructive arguments against the plan, and something you can point to should someone tries to drastically change the path come PR review time after you've already done said work.

If you're not already joined, you can use https://join.slack.com/t/cordova/shared_invite/zt-z70vy6tx-7VNulesO0Qz0Od9QV4tc1Q to join the slack workspace. It's more or less an unofficial channel for realtime chat regarding cordova development. Most development choices and official processes must use the mailing list, but slack is suitable to iron out thoughts or get advice regarding the Apache processes.

GitToTheHub commented 1 month ago

Ok thanks, then I will do a final call.

I'm registered in Slack, but the mailing list is ok for me. And as you wrote, the PR will be a discussion ground about the new integration.

GitToTheHub commented 1 month ago

Hi @breautek,

I found a W3C Community Group Draft Report of the File and Directory Entries API from 7 June 2024 on https://wicg.github.io/entries-api and a documented FileSystemDirectoryEntry on MDN: https://developer.mozilla.org/en-US/docs/Web/API/FileSystemDirectoryEntry. But when I check if there is a FileSystemDirectoryEntry on Android or a requestFileSystem, I cannot find these properties.

Do we have to care about this?

Greetings,

Manuel

breautek commented 1 month ago

I don't think so.

The file plugin has a lot of really old code that obviously has been mangled and manipulated to not only keep up with standard changes previously but to make it work with the underlying android and ios filesystems.

I think adopting any newer standard at this point should probably be done from a clean slate as a new plugin, which might be something that Apache would want to do in the future, but definitely out of scope for right now.

GitToTheHub commented 1 month ago

Ok I understand.

Another thing I noticed is that FileWriter is not specified in https://www.w3.org/TR/FileAPI/. So this can get also Promises?

breautek commented 1 month ago

FileWriter is defined in https://dev.w3.org/2009/dap/file-system/file-writer.html, which is one of the documents that has been discontinued. So on that merit I think we are free to promise-ify the writer.

GitToTheHub commented 1 month ago

Ok I saw this also now. I formatted the readme regarding the specs, so it's clearer which specs are used, which are discontinued and which could be a replacement.