Added JsCreatePromise to JsRT #2581

kfarnung · 2017-02-23T01:12:39Z

No description provided.

liminzhu · 2017-02-23T01:16:24Z

+@curtisman , this is the context #2541

dilijev · 2017-02-23T01:34:20Z

lib/Jsrt/Jsrt.cpp

+
+    return JsNoError;
+  });
+}


nit: indentation level in the rest of this file is 4 spaces.

Good catch, VS doesn't seem to auto-detect indentation.

boingoing

LGTM, should confirm we want the JsCreatePromise API to be a common export.

boingoing · 2017-02-23T01:33:49Z

lib/Jsrt/ChakraCommon.h

+    ///     Requires an active script context.
+    ///     </para>
+    /// </remarks>
+    /// <param name="promise">The new Promise object.</param>


Just a nitpick, this param tag is all one line while the others are split up.

We're inconsistent about this elsewhere in this file as well. I wonder if it has to do with line length and VS doing something automatically with formatting?

boingoing · 2017-02-23T01:36:37Z

lib/Jsrt/Jsrt.cpp

@@ -2866,6 +2867,32 @@ CHAKRA_API JsSetPromiseContinuationCallback(_In_ JsPromiseContinuationCallback p
    /*allowInObjectBeforeCollectCallback*/true);
 }

+CHAKRA_API JsCreatePromise(_Out_ JsValueRef *promise, _Out_ JsValueRef *resolve, _Out_ JsValueRef *reject)
+{
+  return ContextAPIWrapper<true>([&](Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {


Nitpick, indentation here.

liminzhu · 2017-02-23T02:03:29Z

I'm a little confused about what resolveFunction and rejectFunction output are and how I would use them. Would appreciate a short snippet (we need to document the API anyways) 😄 .

Let's say JsCreatePromise is equivalent to new Promise((res, rej) => {...}), would this make more sense?

JsCreatePromise(_In_ JsValueRef executor, _Out_ JsValueRef *promise)

executor could be a native function created by JsCreateFunction.

boingoing · 2017-02-23T02:16:11Z

@liminzhu

I'm a little confused about what resolveFunction and rejectFunction output are and how I would use them.

These are essentially the same functions passed to the executor. Another method to fetch them might be like this:

var resolve, reject;
function executor(res, rej) { resolve = res; reject = rej; };
var promise = new Promise(executor);

Goal would be to allow our native code to resolve the promise whenever it wants to. One use might be an API which reads a file from the network and returns a promise resolved with the contents of that file. Native code can construct the promise using JsCreatePromise and hold-on to resolveFunction for that promise. Assume the native-side performs some processing to fetch the result data and when that is done it can call the resolveFunction with that result data which will handle resolving the promise in the engine. I guess the key thing here is that the native code can resolve the promise in an async way without having to create an executor function which would manually suck out the resolve/reject functions.

liminzhu · 2017-02-23T02:43:24Z

Thanks @boingoing ! Makes much sense now. resolveFunction and rejectFunction are hooks which you can JsCallFucntion upon to resolve/reject a promise natively. I like it and we'd need some proper documentation explaining how this would work. Please move the API to ChakraCore.h to avoid getting shipped to ChakraFull atm.

obastemur · 2017-02-23T12:23:09Z

lib/Jsrt/Jsrt.cpp

+        PARAM_NOT_NULL(resolve);
+        PARAM_NOT_NULL(reject);
+
+        *promise = nullptr;


is this needed? (same applies to two below)

It is not mandatory to have either resolve() or reject() methods FYI.

@obastemur I was just following the pattern of other methods. The idea was to ensure that regardless of how the flow changed we would always have set some value. I realize that in this flow we will always set a value, so I'm fine removing it if others agree.

I don't see a reason to remove this. In this case the compiler will be able to figure out that this operation is not needed -- if the flow ever changes, we are protected.

galvesribeiro · 2017-02-23T12:58:16Z

LGTM. I just would like to mention that it is not required to have both reject() and response() methods. And if possible, it would be good to accept multiple methods accepted because people can catch multiple types of errors on diff methods. For example:

mypromise.then(...).then(...).catch(...).catch(...) <- that is totally viable.

kfarnung · 2017-02-23T18:36:33Z

@galvesribeiro I think you might have it backwards, the resolve and reject parameters are out parameters that will contain pointers to the functions that the native code will call when it has either resolved or rejected the promise. These are equivalent to the parameters that come into the executor method when creating a promise in JavaScript.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise

For continuations you would just use the JsRT functions to get the property (then, catch, etc.) and then call it normally with JsCallFunction.

In C#:

JavaScriptPropertyId thenId = JavaScriptPropertyId.FromString("then");
JavaScriptValue thenFunction = promise.GetProperty(thenId);

JavaScriptValue resolveFunc = JavaScriptValue.CreateFunction(ResolveCallback);
JavaScriptValue rejectFunc = JavaScriptValue.CreateFunction(RejectCallback);
JavaScriptValue thenPromise = thenFunction.CallFunction(promise, resolveFunc, rejectFunc);

Since it's just a JavaScript function, you can choose to leave out the second parameter when calling it.

galvesribeiro · 2017-02-23T18:38:58Z

Oh! Right, I understood it completely wrong, sorry. You are right! Good work!

jianchun · 2017-02-24T17:23:18Z

lib/Jsrt/Jsrt.cpp

+CHAKRA_API JsCreatePromise(_Out_ JsValueRef *promise, _Out_ JsValueRef *resolve, _Out_ JsValueRef *reject)
+{
+    return ContextAPIWrapper<true>([&](Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION_NOT_IMPLEMENTED(scriptContext);


Do we need to implement this for TTD? @mrkmarron

I spoke with @mrkmarron and this was his recommendation. If the user is running TTD and runs into this we'll fail fast and deal with it at that time.

kfarnung · 2017-02-24T19:05:48Z

Closing this one and retargeting to release/2.0 in #2594.

msftclas added the cla-not-required label Feb 23, 2017

kfarnung requested review from digitalinfinity, liminzhu, boingoing and mrkmarron February 23, 2017 01:13

dilijev reviewed Feb 23, 2017

View reviewed changes

boingoing approved these changes Feb 23, 2017

View reviewed changes

kfarnung force-pushed the jsrtpromise branch from d946fce to a7ed8f7 Compare February 23, 2017 01:44

kfarnung force-pushed the jsrtpromise branch from a7ed8f7 to 90dd972 Compare February 23, 2017 02:48

obastemur reviewed Feb 23, 2017

View reviewed changes

kfarnung requested a review from curtisman February 23, 2017 18:53

jaredwy mentioned this pull request Feb 23, 2017

Running native tests xplat #2590

Open

Added JsCreatePromise to JsRT

8d1bb5b

kfarnung force-pushed the jsrtpromise branch from 90dd972 to 8d1bb5b Compare February 23, 2017 21:45

dilijev assigned kfarnung Feb 23, 2017

curtisman approved these changes Feb 24, 2017

View reviewed changes

jianchun reviewed Feb 24, 2017

View reviewed changes

kfarnung changed the base branch from master to release/2.0 February 24, 2017 19:00

kfarnung mentioned this pull request Feb 24, 2017

Added JsCreatePromise to JsRT #2594

Merged

kfarnung closed this Feb 24, 2017

kfarnung deleted the jsrtpromise branch February 24, 2017 19:24

vikrem mentioned this pull request Apr 19, 2018

Native background processing + ChakraCore Promise API #5013

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added JsCreatePromise to JsRT #2581

Added JsCreatePromise to JsRT #2581

kfarnung commented Feb 23, 2017

liminzhu commented Feb 23, 2017

dilijev Feb 23, 2017

kfarnung Feb 23, 2017

boingoing left a comment

boingoing Feb 23, 2017

dilijev Feb 23, 2017

boingoing Feb 23, 2017

liminzhu commented Feb 23, 2017 •

edited

Loading

boingoing commented Feb 23, 2017

liminzhu commented Feb 23, 2017 •

edited

Loading

obastemur Feb 23, 2017

galvesribeiro Feb 23, 2017

kfarnung Feb 23, 2017

dilijev Feb 23, 2017

galvesribeiro commented Feb 23, 2017

kfarnung commented Feb 23, 2017

galvesribeiro commented Feb 23, 2017

jianchun Feb 24, 2017

kfarnung Feb 24, 2017

kfarnung commented Feb 24, 2017

Added JsCreatePromise to JsRT #2581

Added JsCreatePromise to JsRT #2581

Conversation

kfarnung commented Feb 23, 2017

liminzhu commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing 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

liminzhu commented Feb 23, 2017 • edited Loading

boingoing commented Feb 23, 2017

liminzhu commented Feb 23, 2017 • edited Loading

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

galvesribeiro commented Feb 23, 2017

kfarnung commented Feb 23, 2017

galvesribeiro commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfarnung commented Feb 24, 2017

liminzhu commented Feb 23, 2017 •

edited

Loading

liminzhu commented Feb 23, 2017 •

edited

Loading