Skip to content

Conversation

brunolau
Copy link

@brunolau brunolau commented Apr 6, 2022

This adds onProgress callback for uploadFile method. Useful when uploading larger files

This commits adds onProgress callback for uploadFile method. Useful when uploading larger files
}

if (hasProgressHandler) {
request.progress((transferred, total) -> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use Java 8 features (lambda expression in this case), because this will break support for old Cordova projects still configured to run with Java 7.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the lambda, please check if it's ok now

@silkimen
Copy link
Owner

Thank you for providing this feature PR, but please do not use Java 8 features as we are still supporting old Cordova projects running on Java 7.

@brunolau
Copy link
Author

Hi, lambda has been removed

case 'upload':
var fileOptions = helpers.checkUploadFileOptions(options.filePath, options.name);
exec(onSuccess, onFail, 'CordovaHttpPlugin', 'uploadFiles', [url, headers, fileOptions.filePaths, fileOptions.names, options.connectTimeout, options.readTimeout, options.followRedirect, options.responseType, reqId]);
var hasProgressCallback = options.onProgress != null;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to check for falsey value here not for null, because it will be undefined if not provided.

Suggested change
var hasProgressCallback = options.onProgress != null;
var hasProgressCallback = !!options.onProgress;

Comment on lines +185 to +189
if (resp != null && resp.isProgress) {
options.onProgress(resp);
} else {
onSuccess(resp);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please extract this inline handler into a function expression in lines 183ff for making it more readable.


function uploadFile(url, params, headers, filePath, name, success, failure) {
return publicInterface.sendRequest(url, { method: 'upload', params: params, headers: headers, filePath: filePath, name: name }, success, failure);
function uploadFile(url, params, headers, filePath, name, success, failure, progress) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work for the https://github.com/danielsogl/awesome-cordova-plugins/ wrapper? I think it could be a problem but not sure, because usually the last two arguments are meant to be success and fail handlers. Would you please check?
If it causes problems we can keep the short-hand function uploadFile and make this functionality available only via sendRequest.

@silkimen
Copy link
Owner

silkimen commented May 25, 2023

Hi @brunolau, been quite some time but I checked your PR again just now. Left some comments. And also I recognized you could simplify the implementation by not checking if a progress function was supplied in native code, but instead always report the progress to the webview. And the www interface file can check if a progress callback was provided. I think this will be minimal overhead, right? What do you think?

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.

2 participants