-
Notifications
You must be signed in to change notification settings - Fork 465
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
New Promise and Reference docs #243
Conversation
``` | ||
|
||
* `[in] value`: The value which is to be referenced. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should say
The Value object which is to be referenced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @gabrielschulhof could you also take a read through.
@gabrielschulhof ping |
@@ -1,5 +1,70 @@ | |||
# Promise | |||
|
|||
You are reading a draft of the next documentation and it's in continuos update so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: common typo: 'continuos' -> 'continuous'
|
||
```cpp | ||
Promise::Deferred(napi_env env); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gabrielschulhof Do you remember why we expose both a factory and a constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean the fact that we have both a deferred and a promise, then the reason for that is that with V8 N-API cannot resolve a promise coming in from JavaScript, because it doesn't have the corresponding v8::Resolver
.
In reality we could have gone with a single object, as @addaleax points out, but I chose not to because the coincidence between v8::Promise
and v8::Promise::Resolver
is an internal V8 detail and thus not guaranteed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I should have been more clear. It seems like these objects have a lot of different mechanisms for construction. Buffer (which I'm documenting) has four factory methods and two constructors. In the process of writing the documentation the factory methods seem clear, but I haven't quite understood the purpose of the public constructors. They seem like the end up putting the object into a weird (and potentially undesirable state).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's so compilation units written in C can interact with those written in C++?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gabrielschulhof I don't think it was anything like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
* `[in] env`: The `napi_env` environment in which to construct the Reference object. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra whitespaces between list items
``` | ||
|
||
* `[in] env`: The `napi_env` environment in which to create the Deferred object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: looks like other docs use -
(hyphen) for list items
Going to land this and then we can improve through follow on PRs. |
PR-URL: #243 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Landed as 9d38f61 |
PR-URL: nodejs/node-addon-api#243 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
PR-URL: nodejs/node-addon-api#243 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
PR-URL: nodejs/node-addon-api#243 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
PR-URL: nodejs/node-addon-api#243 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Continuing work for nodejs/abi-stable-node#280