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

Promises #637

Open
GitToTheHub opened this issue Sep 29, 2024 · 10 comments
Open

Promises #637

GitToTheHub opened this issue Sep 29, 2024 · 10 comments

Comments

@GitToTheHub
Copy link
Contributor

GitToTheHub commented Sep 29, 2024

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:

  • For Android the minimum supported Android version is 7.0 since cordova-android 12. Promises are supported since Version 4.4.4
  • For iOS, WKWebView is the required minimum. Promises are supported since iOS 8.

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

@GitToTheHub GitToTheHub changed the title Talk about Promises Promises Sep 29, 2024
@breautek
Copy link
Contributor

breautek commented Oct 2, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@breautek
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

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
Copy link
Contributor Author

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

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
Copy link
Contributor Author

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.

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

No branches or pull requests

2 participants