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

feat: native Promise APIs #10554

Merged
merged 10 commits into from Dec 9, 2020
Merged

feat: native Promise APIs #10554

merged 10 commits into from Dec 9, 2020

Conversation

drauggres
Copy link
Contributor

JIRA: https://jira.appcelerator.org/browse/TIMOB-24549

I suggest to reopen TIMOB-24549.
This PR introduces an interface for creating asynchronous methods (for native Android modules) that return ES6 Promise.

Sample application:

let win = Ti.UI.createWindow({
  title: 'Promise API POC',
  layout: 'vertical',
  backgroundColor: '#fff'
});

let btn1 = Ti.UI.createButton({title: 'Promise'});
btn1.addEventListener('click', (event) => {
  console.log(event.type, event.source.title);
  Ti.Filesystem.requestStoragePermissions()
    .then(function(result) {
        console.log(event.source.title, JSON.stringify(result));
    })
    .catch(function(error) {
        console.error(event.source.title, JSON.stringify(error));
    });
});

let btn2 = Ti.UI.createButton({title: 'async/await'});
btn2.addEventListener('click', onClick);

async function onClick(event) {
  console.log(event.type, event.source.title);
  try {
    let result = await Ti.Filesystem.requestStoragePermissions();
    console.log(event.source.title, JSON.stringify(result));
  } catch (error) {
    console.error(event.source.title, JSON.stringify(error));
  }
}

let btn3 = Ti.UI.createButton({title: 'callback'});
btn3.addEventListener('click', function(e) {
  console.log(e.type, e.source.title);
  Ti.Filesystem.requestStoragePermissions(function(result) {
      if (result && result.success) {
        console.log(e.source.title, JSON.stringify(result));
      } else {
        console.error(e.source.title, JSON.stringify(result));
      }
  })
});

win.add(btn1);
win.add(btn2);
win.add(btn3);

win.open();

@build build added this to the 8.0.0 milestone Dec 20, 2018
@build build requested a review from a team December 20, 2018 12:42
@brentonhouse
Copy link
Contributor

Thanks for the contribution @drauggres !

@drauggres
Copy link
Contributor Author

drauggres commented Jan 9, 2019

How to use it

Java:

  1. Create void method on KrollProxy and annotate it with @Kroll.method
  2. Add KrollPromise as the last argument to this method
  3. Call resolve or reject on KrollPromise instance when async result is ready

Generated native method will not be void, but will return ES6 Promise and also will pass to java method newly created KrollPromise object.

JS:

  1. Call the method, receive Promise as the result
  2. Await for Promise fulfillment (call then/catch or use async/await)

UPD (04.01.2021): strike out outdated

@sgtcoolguy
Copy link
Contributor

@drauggres I would absolutely love to get this in so we can offer Promise based APIs for any async methods. However, I'm wary of doing so without some tests in our suite for it. We can't test permission requests in our unit test suite, so the changes here aren't something we can test. But maybe if you can modify our WebViewProxy.java to support Promises in place of a callback function?

Or does that not work because it's return type is Object? Maybe GeolocationModule.forwardGeocoder() and reverseGeocoder()?

@sgtcoolguy
Copy link
Contributor

Another general idea: This clearly coincides with usage of KrollFunction in the APIs. Maybe there's some way to unify the idea of an optional callback function which if not specified becomes a Promise?

I dunno, just throwing ideas out there. We clearly have some funky legacy APIs that can be run sync or async and may return values in sync mode so the idea of enforcing a void return type for this to happen might not cover all cases.

@garymathews
Copy link
Contributor

@sgtcoolguy I agree, similar to the behaviour I have in ti.es6 https://github.com/appcelerator/ti.es6/blob/master/plugins/ti.es6/1.3.0/Titanium/Geolocation.js#L352

I would prefer we have this backwards compatibility.

@jquick-axway
Copy link
Contributor

Instead of implementing promises natively per platform, we can keep the native APIs as-is (still using KrollFunction), and instead wrap/override these APIs using our core JavaScript code which is shared on all platforms. That way we can implement promise support on all platforms (Android, iOS, Windows) in 1 shot.
https://github.com/appcelerator/titanium_mobile/tree/master/common/Resources

Something like the below...

var oldRequestMethod = Ti.Filesystem.requestStoragePermissions;
Ti.Filesystem.requestStoragePermissions = function(callback) {
	// Don't use a promise if a callback was given. (Legacy behavior.)
	if (callback) {
		oldRequestMethod(callback);
		return;
	}

	// Callback not provided. Use promises solution instead.
	return new Promise(function(resolve, reject) {
		oldRequestMethod(function(result) {
			if (result.success) {
				resolve(result);
			} else {
				reject(result);
			}
		});
	});
};

The only issues that are blocking us from doing the above are:

  • iOS 9 doesn't support promises if transpile/polyfill is disabled.
  • iOS doesn't allow us to overwrite core module JS methods.
  • On Android, every required-in JS file gets their own copy of Titanium's core modules. So while we can override JS core module methods on Android, it's not a global change.

@drauggres
Copy link
Contributor Author

drauggres commented Jan 10, 2019

@jquick-axway This is not about migrating permissions-requests-API to promises. What I want is to have ability to create native modules with modern API. Of course we can wrap any callbacks into promises, there are already projects doing this.

@sgtcoolguy

Another general idea: This clearly coincides with usage of KrollFunction in the APIs. Maybe there's some way to unify the idea of an optional callback function which if not specified becomes a Promise?

I would prefer to not closely integrate KrollFunction and KrollPromise. It works fine like this. Changes in PermReqAPI allow to use it both ways without breaking anything (sample is in the PR description). Somedays some new APIs will be created and there are no reason for them to be callback-based.

I dunno, just throwing ideas out there. We clearly have some funky legacy APIs that can be run sync or async and may return values in sync mode so the idea of enforcing a void return type for this to happen might not cover all cases.

Yeah, there are always some legacy, but, once again, this PR is not about changing any current APIs. The second commit here is mostly for example.

Maybe GeolocationModule.forwardGeocoder() and reverseGeocoder()?

OK, I'll modify this methods and add tests soon.

I was thinking about creating a module for testing interface generation itself with method/tests like:

  • resolve/reject promise with primitive values
  • resolve/reject promise with Object
  • resolve/reject promise with Array
  • etc.
    I don't think it would be right to create it in core, but also it looks like there is no easy way to put an additional module to test suite. Any ideas on this?

@drauggres
Copy link
Contributor Author

Maybe GeolocationModule.forwardGeocoder() and reverseGeocoder()?

OK, I'll modify this methods and add tests soon.

Done.

@sgtcoolguy sgtcoolguy modified the milestones: 8.0.0, 8.1.0 Jan 14, 2019
@sgtcoolguy
Copy link
Contributor

We're planning to do 8.0.0 RC this week, so just looking at our timeline this will definitely need to get bumped to 8.1.0 - updated the milestone for now.

@sgtcoolguy sgtcoolguy modified the milestones: 8.1.0, 8.2.0 Jun 3, 2019
@sgtcoolguy sgtcoolguy modified the milestones: 8.2.0, 8.3.0 Sep 5, 2019
@build
Copy link
Contributor

build commented Sep 9, 2019

Warnings
⚠️ It looks like you have modified the TopTiModule.m file. Are you sure you meant to do that?
Messages
📖

💾 Here's the generated SDK zipfile.

📖 🎉 Another contribution from our awesome community member, drauggres! Thanks again for helping us make Titanium SDK better. 👍
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 12932 tests are passing.
(There are 897 skipped tests not included in that total)

Generated by 🚫 dangerJS against 35bc9c4

@sgtcoolguy sgtcoolguy removed this from the 8.3.0 milestone Dec 10, 2019
@build build added this to the 9.1.0 milestone Jul 9, 2020
@garymathews
Copy link
Contributor

garymathews commented Dec 3, 2020

Not all APIs containing callbacks have been converted to promises. Here's a list of all current APIs that contain callbacks:

@sgtcoolguy sgtcoolguy force-pushed the TIMOB-24549 branch 2 times, most recently from 948da56 to f8ad156 Compare December 7, 2020 19:55
@sgtcoolguy
Copy link
Contributor

@garymathews This PR was really intended to enable native promises and modified some APIs as a proof of concept. But yeah we should look to enable returning promises for all async APIs we can once this is in.

Copy link
Contributor

@garymathews garymathews left a comment

Choose a reason for hiding this comment

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

CR: PASS

@jquick-axway
Copy link
Contributor

@sgtcoolguy, regarding our direct V8Promise usage, normally we would use the Kroll* prefixed classes instead. I doubt we would ever switch JS engines to take advantage of this design, but we may want to change things over to use KrollPromise instead for consistency.

Java 8 supports static methods within an interface. So, it could be set up to be used like this...

KrollPromise promise = KrollPromise.create((promise) -> {
	if (successful) {
		promise.resolve("I did it.");
	} else {
		promise.reject("I failed.");
	}
});
return promise;

And ideally you should be able to use it without a callback. This may make it easier to integrate into other APIs.
(Do we even need the callback? It would be easier to integrate into our existing code without it.)

KrollPromise promise = KrollPromise.create();
if (successful) {
	promise.resolve("I did it.");
} else {
	promise.reject("I failed.");
}
return promise;

We could implement KrollPromise as shown below. The key thing here is to create the V8Promise via an abstract KrollRuntime.createPromise() method that the V8Runtime derived class can implement.

public interface KrollPromise {
	interface OnExecuteCallback {
		void onExecute(KrollPromise promise);
	}

	void resolve(Object value);
	void reject(Object value);

	static KrollPromise create() {
		return create(null);
	}

	static KrollPromise create(OnExecuteCallback callback) {
		KrollPromise promise = KrollRuntime.getInstance().createPromise();
		if (promise == null) {
			promise = new MockPromise();
		}
		if (callback != null) {
			TiMessenger.postOnRuntime(() -> {
				callback.onExecute(promise);
			});
		}
		return promise;
	}

	private static class MockPromise implements KrollPromise {
		@Override
		public void resolve(Object value) {}
		@Override
		public void reject(Object value) {}
	}
}

public abstract class KrollRuntime {
	public abstract KrollPromise createPromise();
}

public class V8Runtime extends KrollRuntime {
	@Override
	public KrollPromise createPromise() {
		if (KrollRuntime.isDisposed()) {
			return null;
		}
		return new V8Promsie();
	}
}

I'm not trying to cause trouble. Just tossing in some ideas here.

@sgtcoolguy
Copy link
Contributor

@jquick-axway re: the callback - yes, it's definitely needed, otherwise there's no way to ensure the promise "body" is going to be async. If we simply create a promise and keep on executing, we're running the code sync but pretending it's async - the promise will already have rejected/resolved by the time the method returns it.

I do see your point that we have typically hid the JS-engine specific subclass vs the Kroll abstract type so we could swap engines, though as you mentioned it's highly unlikely we're swapping engines again at this point (this is a relic from then they swapped from rhino to v8). I'll see if I can hide the implementation class as you suggest.

@build build added the docs label Dec 9, 2020
@sgtcoolguy sgtcoolguy merged commit dd34c16 into tidev:master Dec 9, 2020
@sgtcoolguy
Copy link
Contributor

For reference, I've opened https://jira.appcelerator.org/browse/TIMOB-28279 to track enabling more of our APIs to return Promises.

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

Successfully merging this pull request may close these issues.

None yet

8 participants