n-api: add napi_get_promise_{state,result}

[devsnek]

@nodejs/n-api

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[devsnek]

CI https://ci.nodejs.org/job/node-test-pull-request/14775/

[addaleax]

@nodejs/n-api We probably want to have a very conscious decision on this, because this is intentionally not part of the standard and intentionally not exposed in Node.js.

It might be helpful to know how this benefits addons over using .then() for Promises?

[gabrielschulhof]

Typo

[gabrielschulhof]

For example, in its current state, node-chakracore will be unable to implement this without significant effort, because the V8 shim is only able to retrieve the result and state of those promises that were created natively.

The test:

#include "node.h"

using namespace v8;

static void method(const FunctionCallbackInfo<Value>& info) {
  info.GetReturnValue().Set(info[0].As<Promise>()->Result());
}

static void Init(Local<Object> exports, Local<Value> module) {
  Isolate* isolate = Isolate::GetCurrent();
  Local<Context> context = isolate->GetCurrentContext();
  Local<FunctionTemplate> tpl = FunctionTemplate::New(isolate, method);
  exports->Set(context,
      String::NewFromUtf8(isolate, "method", NewStringType::kNormal)
          .ToLocalChecked(),
    tpl->GetFunction()).FromJust();
}

NODE_MODULE(NODE_GYP_MODULE_NAME, Init)

and

const binding = require('./build/Release/test_promise');
console.log(binding.method(Promise.resolve(5)));

produces 5 when running with node-v8, and undefined when running with node-chakracore.

@kfarnung @digitalinfinity

[devsnek]

that's an unfortunate and strange limitation, I suppose this can be blocked until that is fixed? I would imagine that also breaks util.inspect

[devsnek]

@addaleax I can't predict all use cases but at least one I can think of is debugging (like util.inspect)

[jasnell]

Explicitly marking this blocked based on the conversation. I'm generally ok with napi_get_promise_state but the napi_get_promise_result is questionable.

[addaleax]

I can't predict all use cases but at least one I can think of is debugging (like util.inspect)

Yeah, like you’re saying, we have util.inspect() for this. :) Or are you talking about debugging the addon itself? Because in that case I think we might want to go another route and provide some kind of napi_inspect method?

[mhdawson]

This is missing the "Returns" to match the standard doc format.

It would also be good to add an explanation of when/why you use the method

[mhdawson]

same comment as for previous method.

[kfarnung]

What does this value contain if the promise hasn't been rejected or resolved yet?

[devsnek]

good question, I think I'll need to add an error case for that.

[kfarnung]

Can we add a test for an unresolved promise?

[kfarnung]

@gabrielschulhof @devsnek It's not that ChakraCore can't do it, we just don't currently expose an API for doing so. We just haven't had a specific need for it in the past. I'm looking at what it would take to add that functionality, the only thing I'm still iterating on is what the error conditions are (e.g. what happens when the Promise is still pending and the result is requested).

/cc @MSLaguana @digitalinfinity

[kfarnung]

FYI: I believe the repro that @gabrielschulhof had should be fixed when nodejs/node-chakracore#537 lands.

[mhdawson]

I agree on making a conscious decision, that was part of the reason I was suggesting additional doc as to why you would use these functions. With it not being part of the standard I think we should only add them if we have a well-documented need and no reasonable way to work around it. Keeping the API surface to a smaller set will help us in terms of maintenance going forward as well as being able to support different JavaScript Engines.

[mhdawson]

@devsnek any chance you can make the next N-API meeting (Thrusdays at 1:30 EST). We can talk about this more. In addition, from your recent activity, it looks like you have time to contribute to N-API and we can sync you up with the work we are currently focussing on.

[devsnek]

@mhdawson i'd be happy to show up 👍

[mhdawson]

@devsnek the meeting is on the foundation calendar, and it includes the invite info. We should be using the primary meeting (zoom) next week: https://nodejs.org/calendar

[gabrielschulhof]

@kfarnung I guess this page is seriously out of date: https://github.com/Microsoft/ChakraCore/wiki/JavaScript-Runtime-%28JSRT%29-Reference - it mentions neither JsGetPromiseState nor JsGetPromiseResult.

[digitalinfinity]

That's because Kyle just implemented them yesterday 😄

[MSLaguana]

And there is a PR to address it too: microsoft/ChakraCore-wiki#54

[BridgeAR]

Any update here?

[devsnek]

i was planning to suggest an api for converting a napi_value to an engine handle (like v8::Value) at some point to replace this but never got around to it.

[gabrielschulhof]

@devsnek I suppose we could provide facilities for mixing N-API with the underlying engine, but they would have to be such behind preprocessor conditionals that vary by the engine.

[devsnek]

@gabrielschulhof yeah i think that's a given, but it would provide (imo) a nicer experience than forcing engines to expose apis that don't relate directly to the spec.

[MSLaguana]

Wouldn't mixing N-API with engine-specific things end up removing the benefits of N-API? E.g. if you try to do something with an engine-specific handle, that is no longer guaranteed to work across multiple versions of that engine/node. At that point, aren't you essentially back to linking against a particular node binary?

[devsnek]

@MSLaguana yes you're opting out of any sort of stability, i imagine that library authors probably wouldn't want to use a feature like that.

[gabrielschulhof]

@devsnek IMO adding this is only beneficial if it can spur adoption of N-API by providing temporary relief for projects that find the N-API surface lacking and with a clear understanding that this removes the promise of never having to compile again.

Within that scope, though, it might help those projects move most of their code base to N-API, which in turn might help them present clear use cases, backed by code, of what is missing from N-API. As a result, it might also help them contribute to the discussion of what future N-APIs should look like.

Any such bridge to native couldn't be written into node_api.h though, because that file has to compile using a plain C compiler.

[devsnek]

@gabrielschulhof yeah, i was planning to put it in node.h