Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

CB-12815: (ios) Fix bug nativeCallback not executed when app is in background #49

Merged
merged 2 commits into from
Aug 23, 2018

Conversation

Nauzer
Copy link
Contributor

@Nauzer Nauzer commented Aug 20, 2018

Platforms affected

iOS

What does this PR do?

Fix the situation that Cordova native callbacks are not executed when the app is backgrounded of the Main UIView is not visible.

What testing has been done on this change?

Testing of native callback functionality being executed.

Checklist

  • Reported an issue in the JIRA database
    https://issues.apache.org/jira/browse/CB-12815
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.
    N/A IMO

@shazron
Copy link
Member

shazron commented Aug 21, 2018

Thanks! Can you update the version to "1.2.0-dev" in package.json?

@shazron shazron self-assigned this Aug 21, 2018
Add back eslint ignore line
@Nauzer
Copy link
Contributor Author

Nauzer commented Aug 22, 2018

Updated version to 1.2.0-dev and added back the eslint ignore rule for the line (based on original code)

@shazron shazron merged commit bf0f236 into apache:master Aug 23, 2018
@shazron
Copy link
Member

shazron commented Aug 23, 2018

Promises work since iOS 8 and WKWebView has only been introduced since iOS 8, so this is 100% safe

@jcesarmobile
Copy link
Member

Why is the promise needed? isn't it enough just to remove the setTimeout?

@shazron
Copy link
Member

shazron commented Sep 12, 2018

@jcesarmobile see the JIRA Issue. process.nextTick, Promises and setTimeout put the code into a the queue for the next process tick. This preserves the existing setTimeout behaviour that Apple has apparently suppressed for setTimeout.

@awerlang
Copy link

awerlang commented Sep 19, 2018

@shazron

@jcesarmobile see the JIRA Issue. process.nextTick, Promises and setTimeout put the code into a the queue for the next process tick. This preserves the existing setTimeout behaviour that Apple has apparently suppressed for setTimeout.

This is incorrect, the behavior is not preserved.

Keep in mind that the code:

Promise.resolve(cordova.callbackFromNative(callbackId, success, status, args, keepCallback));

is the same as:

const cb = cordova.callbackFromNative(callbackId, success, status, args, keepCallback);
Promise.resolve(cb);

And equivalent to:

const cb = cordova.callbackFromNative(callbackId, success, status, args, keepCallback);
setTimeout(function() {}, 0);

To maintain the same behavior do this:

Promise.resolve().then(function () {
    cordova.callbackFromNative(callbackId, success, status, args, keepCallback);
});

jrunkel pushed a commit to jrunkel/cordova-plugin-wkwebview-engine that referenced this pull request Dec 7, 2018
…ckground (apache#49)

* CB-12815: (ios) Fix bug nativeCallback not executed when app is in background

* Update package.json version to 1.2.0-dev
Add back eslint ignore line
@alexhisen
Copy link
Contributor

alexhisen commented Feb 15, 2019

@shazron Can you update the code change to:

Promise.resolve().then(function () {
    cordova.callbackFromNative(callbackId, success, status, args, keepCallback);
});

as per @awerlang and perhaps publish 1.2.x to npm?
This fix is definitely needed to run code for a background fetch.

@janpio
Copy link
Member

janpio commented Feb 15, 2019

Please open a Pull Request @alexhisen with those changes if you want to get this merged. Thanks.

@janpio
Copy link
Member

janpio commented Jun 27, 2019

I am currently working on a cordova-plugin-wkwebview-engine release (see apache/cordova#104 (comment)). Can someone here help me understand why this PR required a minor version bump? To me this looks like a plain bugfix that could be fine to be released in 1.1.5 - or am I missing something?

@shazron
Copy link
Member

shazron commented Jun 28, 2019

@janpio I forget - but it was probably because the behaviour of this functionality potentially has changed and we wanted to signal it as such. Feel free to revert the version change.

@janpio
Copy link
Member

janpio commented Jun 28, 2019

Ok thanks, so I will consider this optional and decide after I looked through all the changes.

@janpio
Copy link
Member

janpio commented Jun 28, 2019

Turns out #69 also adds functionality, so a minor bump for the next release is totally justified.

@donkeytronkilla
Copy link

Can someone explain why moving the call to cordova.callbackFromNative to the next tick is necessary? I understand that this what Promise.resolve does, and that setTimeout in the original code seemed to get throttled by the iOS updates, but I don't understand why this was being queued for the next tick in the first place. What are the problems it was intended to solve? Is there some simple example interaction someone could describe to make it clear why cordova.callbackFromNative should be queued until the next tick?

@donkeytronkilla
Copy link

I read the original bug report on the JIRA, but there it also seemed like executing cordova.callbackFromNative synchronously would have solved the problem that was originally motivating this fix.

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

Successfully merging this pull request may close these issues.

None yet

7 participants