Add promise C API #1796

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

jerry-core/jerry.c

+jerry_value_is_promise (const jerry_value_t value) /**< api value */
+{
+  jerry_assert_api_available ();
+#ifndef CONFIG_DISABLE_ES2015_PROMISE_BUILTIN


newline before ifdef

Please add newlines before other ifdefs in this file.

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

tests/unit-core/test-promise.c

+
+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

tests/unit-core/test-promise.c

+
+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-core/jerry.c

+#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 */
+  return ecma_raise_syntax_error (ECMA_ERR_MSG ("Promise profile has been disabled."));


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

jerry-core/jerry.c

+static jerry_value_t
+jerry_resolve_or_reject_promise (jerry_value_t promise,
+                                 jerry_value_t argument,
+                                 bool is_resolve)


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

docs/02.API-REFERENCE.md

+
+```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

docs/02.API-REFERENCE.md

@@ -1980,6 +2021,64 @@ jerry_value_to_string (const jerry_value_t value);
 - [jerry_value_to_primitive](#jerry_value_to_primitive)


+# Functions for promise value


Functions for promise objects.

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

docs/02.API-REFERENCE.md

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


resolved or rejected

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

docs/02.API-REFERENCE.md

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


resolved/rejected

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

docs/02.API-REFERENCE.md

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


newly created promise

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

jerry-core/ecma/operations/ecma-promise-object.h

+typedef enum
+{
+  ECMA_PROMISE_EXECUTOR_FUNCTION, /**< the executor is a function, it is for the usual constructor */
+  ECMA_PROMISE_EXECUTOR_OBJECT, /**< the executor is a object, it is for the `then` routine */


the executor is an object

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

jerry-core/jerry.c

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


Only one newline.

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

jerry-core/jerry.c

+ *         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

tests/unit-core/test-promise.c

+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

tests/unit-core/test-promise.c

+} /* 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

tests/unit-core/test-promise.c

+} /* 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

tests/unit-core/CMakeLists.txt

-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 zidong.jiang@intel.com

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 zidong.jiang@intel.com

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add promise C API #1796

Add promise C API #1796

jiangzidong commented May 2, 2017 •

edited

Loading

zherczeg left a comment

zherczeg May 2, 2017

zherczeg May 2, 2017

zherczeg May 2, 2017

zherczeg May 2, 2017

martijnthe May 2, 2017

martijnthe May 2, 2017

jiangzidong May 2, 2017

martijnthe May 2, 2017

jiangzidong May 2, 2017

martijnthe commented May 2, 2017

zherczeg May 3, 2017

martijnthe May 3, 2017

martijnthe May 3, 2017

jiangzidong May 3, 2017

zherczeg left a comment

zherczeg May 3, 2017

zherczeg May 3, 2017

zherczeg May 3, 2017

zherczeg May 3, 2017

zherczeg May 3, 2017

zherczeg May 3, 2017

LaszloLango May 4, 2017

LaszloLango May 4, 2017

LaszloLango May 4, 2017

LaszloLango May 4, 2017

LaszloLango May 4, 2017

jiangzidong May 4, 2017

jiangzidong May 4, 2017

LaszloLango May 4, 2017

LaszloLango left a comment

martijnthe commented May 4, 2017

		@@ -1980,6 +2021,64 @@ jerry_value_to_string (const jerry_value_t value);
		- [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 */


		/**

Add promise C API #1796

Add promise C API #1796

Conversation

jiangzidong commented May 2, 2017 • edited Loading

zherczeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnthe commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zherczeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaszloLango left a comment

Choose a reason for hiding this comment

martijnthe commented May 4, 2017

jiangzidong commented May 2, 2017 •

edited

Loading