Add promise C API by jiangzidong · Pull Request #1796 · jerryscript-project/jerryscript

jiangzidong · 2017-05-02T06:04:17Z

related issue: 1794

add api:

bool jerry_value_is_promise (const jerry_value_t value);
jerry_value_t jerry_create_promise (void);
jerry_value_t jerry_resolve_promise (jerry_value_t promise, jerry_value_t argument);
jerry_value_t jerry_reject_promise (jerry_value_t promise, jerry_value_t reason);

unittest support es2015-subset profile
doc will be added later when we reach the consensus

zherczeg

Good patch! Please add documentation for the new functions.

zherczeg · 2017-05-02T06:39:26Z

+bool
+jerry_value_is_promise (const jerry_value_t value) /**< api value */
+{
+  jerry_assert_api_available ();


newline before ifdef

Please add newlines before other ifdefs in this file.

zherczeg · 2017-05-02T06:42:48Z

+
+static jerry_value_t
+create_promise1_handler (const jerry_value_t func_obj_val __attribute__((unused)), /**< function object */
+                        const jerry_value_t this_val __attribute__((unused)), /**< this value */


One more space needed.

zherczeg · 2017-05-02T06:44:04Z

+
+static jerry_value_t
+create_promise2_handler (const jerry_value_t func_obj_val __attribute__((unused)), /**< function object */
+                        const jerry_value_t this_val __attribute__((unused)), /**< this value */


One more space needed.

martijnthe · 2017-05-02T09:35:24Z

+  jerry_assert_api_available ();
+#ifndef CONFIG_DISABLE_ES2015_PROMISE_BUILTIN
+  return ecma_op_create_promise_object (ecma_make_simple_value (ECMA_SIMPLE_VALUE_EMPTY), ECMA_PROMISE_EXECUTOR_EMPTY);
+#else /* CONFIG_DISABLE_ES2015_PROMISE_BUILTIN */


Maybe better to exclude the implementation so you get a linker error?
I don't know... returning an error adds code space.

martijnthe · 2017-05-02T09:37:39Z

+ */
+static jerry_value_t
+jerry_resolve_or_reject_promise (jerry_value_t promise,
+                                 jerry_value_t argument,


I think it makes sense to expose something like this in the public API.

I think people will be writing code like in this example a lot, so you naturally have a bool to make the decision whether resolve or reject should be called.

In an earlier sketch, I used an enum, I think that has a bit better readability vs bool, but I'm not feeling strongly about it.

My thought is the C api should be similar with js api, so it will be easy to use. In JS world, usually the code is like

var p = new Promise(function(resolve, reject) { .... if (some condition) { var reason = .....//prepare the reason reject (reason) } var argument = .... //prepare the argument resolve(argument) })

in your example,

static void my_get_batt_info_cb (const batt_info_t *info, void *cb_data) { jerry_value_t promise = (jerry_value_t)(uintptr_t) cb_data; // do stuff with *info and assign result + should_call_resolve: jerry_value_t result = ...; bool should_call_resolve = ...; jerry_call_resolve_reject(promise, result, should_call_resolve); jerry_value_release(promise); }

there should be some if-else or branch code in result = and should_call_resolve = , e.g.

if (some condition) { result = some_error_reason; should_call_resolve = false; } else {result = some_argument; should_call_resolve = true;}

why just directly call the resolve/reject in the branch?

why just directly call the resolve/reject in the branch?

Code space savings. You'd have 2x function calls vs 1x.
Often you already have the bool should_call_resolve one way or another, for example in your snippet, there is some_condition. In this case, some_condition == !should_call_resolve.

Ok, it makes sense :)

martijnthe · 2017-05-02T09:38:33Z

@jprestwo any feedback on this API?

zherczeg · 2017-05-03T07:44:38Z

+
+```c
+jerry_value_t
+jerry_promise_get_resolve (jerry_value_t promise)


What I don't understand is that we have one function to resolve/reject, but two functions for getting functions. I think it would be better to have a boolean parameter here as well.

My gut says that if your project needs access to the functions, you probably are going to need both.

Would it make sense to have:

bool jerry_promise_get_resolve ( jerry_value_t promise, jerry_value_t *resolve_out_p, jerry_value_t *reject_out_p)

Hmm I wonder if we're adding the resolve/reject getter prematurely...
Maybe just remove it and get on with life until someone comes along that has a clear use for it?

zherczeg

LGTM after some style fixes.

zherczeg · 2017-05-03T12:20:21Z

 - [jerry_value_to_primitive](#jerry_value_to_primitive)


+# Functions for promise value


Functions for promise objects.

zherczeg · 2017-05-03T12:21:03Z

+
+  ...
+
+  bool is_resolve = ... // whether the promise should be resolve or reject


resolved or rejected

zherczeg · 2017-05-03T12:21:33Z

+
+**Summary**
+
+Create an empty Promise object which can be resolve/reject later


resolved/rejected

zherczeg · 2017-05-03T12:21:54Z

+jerry_create_promise (void)
+```
+
+- return value - value of the created promise


newly created promise

zherczeg · 2017-05-03T12:24:27Z

+ */
+typedef enum
+{
+  ECMA_PROMISE_EXECUTOR_FUNCTION, /**< the executor is a function, it is for the usual constructor */


the executor is an object

zherczeg · 2017-05-03T12:25:06Z

+} /* jerry_value_is_promise */
+
+
+/**


Only one newline.

LaszloLango · 2017-05-04T07:01:28Z

+ *         false - otherwise
+ */
+bool
+jerry_value_is_promise (const jerry_value_t value) /**< api value */


Please use the same order of functions as 'jerryscript.h'.

LaszloLango · 2017-05-04T07:12:34Z

+const jerry_char_t s2[] = "rejected";
+
+static jerry_value_t
+create_promise1_handler (const jerry_value_t func_obj_val __attribute__((unused)), /**< function object */


JERRY_UNUSED must be available here since test-common.h includes jrt.h

static jerry_value_t create_promise1_handler (const jerry_value_t func_obj_val , /**< function object */ const jerry_value_t this_val, /**< this value */ const jerry_value_t args_p[], /**< arguments list */ const jerry_length_t args_cnt) /**< arguments length */ { JERRY_UNUSED (func_obj_val); JERRY_UNUSED (this_val); JERRY_UNUSED (args_p); JERRY_UNUSED (args_cnt); ...

LaszloLango · 2017-05-04T07:12:41Z

+} /* create_promise1_handler */
+
+static jerry_value_t
+create_promise2_handler (const jerry_value_t func_obj_val __attribute__((unused)), /**< function object */


LaszloLango · 2017-05-04T07:12:49Z

+} /* create_promise2_handler */
+
+static jerry_value_t
+assert_handler (const jerry_value_t func_obj_val __attribute__((unused)), /**< function object */


LaszloLango · 2017-05-04T07:14:59Z

-if(NOT ${FEATURE_PROFILE} STREQUAL "${CMAKE_SOURCE_DIR}/jerry-core/profiles/es5.1.profile")
-  message(FATAL_ERROR "FEATURE_PROFILE='${FEATURE_PROFILE}' isn't supported with UNITTESTS=ON")
+if(${FEATURE_PROFILE} STREQUAL "${CMAKE_SOURCE_DIR}/jerry-core/profiles/minimal.profile")
+  message(FATAL_ERROR "minimal profile isn't supported in unit-core")


Is this change related to this this PR? It looks to me if promise is not available than the test-promise returns zero, which is good. Am I missing something?

Before this change, I cannot ./tools/build.py --unittest --profile=es2015-subset, because it's profile is not es5.1.

And also I changed the run-test.py, now es2015-subset profile is passed to unittest

# Test options for unittests JERRY_UNITTESTS_OPTIONS = [ Options('unittests', ['--unittests', '--error-messages=on', '--snapshot-save=on', '--snapshot-exec=on', '--vm-exec-stop=on', '--profile=es2015-subset']), Options('unittests-debug', ['--unittests', '--debug', '--error-messages=on', '--snapshot-save=on', '--snapshot-exec=on', '--vm-exec-stop=on', '--profile=es2015-subset']) ]

I see. Thanks for the explanation.

related issue: 1794 JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

LaszloLango

Thanks for the update. LGTM

martijnthe · 2017-05-04T08:16:24Z

💯

jiangzidong mentioned this pull request May 2, 2017

Add C API for Promise #1794

Closed

zherczeg requested changes May 2, 2017

View reviewed changes

zherczeg mentioned this pull request May 2, 2017

Move jerry-core API implementations and headers into dedicated subdirectories #1793

Merged

martijnthe reviewed May 2, 2017

View reviewed changes

zherczeg reviewed May 3, 2017

View reviewed changes

jiangzidong force-pushed the cpromise branch from a4ca366 to 6482201 Compare May 3, 2017 10:25

zherczeg approved these changes May 3, 2017

View reviewed changes

jiangzidong force-pushed the cpromise branch from 6482201 to af07a21 Compare May 3, 2017 12:44

martijnthe mentioned this pull request May 4, 2017

Promise? nodejs/abi-stable-node#242

Closed

LaszloLango requested changes May 4, 2017

View reviewed changes

jiangzidong force-pushed the cpromise branch from af07a21 to 3e93981 Compare May 4, 2017 07:35

Add promise C API

3e93981

related issue: 1794 JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

LaszloLango approved these changes May 4, 2017

View reviewed changes

jiangzidong merged commit ede1383 into jerryscript-project:master May 4, 2017

jiangzidong deleted the cpromise branch May 4, 2017 08:13

		- [jerry_value_to_primitive](#jerry_value_to_primitive)


		# Functions for promise value


		...

		bool is_resolve = ... // whether the promise should be resolve or reject


		Summary

		Create an empty Promise object which can be resolve/reject later

		} /* jerry_value_is_promise */


		/**

Conversation

jiangzidong commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martijnthe commented May 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

martijnthe commented May 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jiangzidong commented May 2, 2017 •

edited

Loading