AsyncWorker, ObjectReference, FunctionReference classes #3
Conversation
napi-inl.h
Outdated
|
|
||
| inline Object AsyncWorker::Persistent() { | ||
| return _persistent.Value(); | ||
| } |
There was a problem hiding this comment.
I find the name slightly confusing. I would expect a method called Persistent to return a Persistent<Object> &.
There was a problem hiding this comment.
This is a replacement for the SaveToPersistent and GetFromPersistent overloaded methods on Nan::AsyncWorker. However I've been thinking this might not be the best design. The problem with the single method for retrieving the Object from the persistent reference is that it requires the caller to have a HandleScope to ensure the Object handle gets cleaned up efficiently. The other methods were verbose but had the advantage of managing the HandleScope automatically.
There was a problem hiding this comment.
Just call them Set and Get. Think of AsyncWorker as an extension of Object, although it is a composition.
There was a problem hiding this comment.
I think I like having the Set and Get as opposed to just returning the persistentl. I'm assuming they were in NaN because they were used frequently enough that having the code once on AsyncWorker made sense.
napi-inl.h
Outdated
| return _persistent.Value(); | ||
| } | ||
|
|
||
| inline void AsyncWorker::HandleOKCallback() { |
There was a problem hiding this comment.
Would it not be better to drop the Handle-prefix on these method names? The Callback suffix also feels slightly redundant. How about something like OnOk, OnError, OnExecute, which would fit the other names?
There was a problem hiding this comment.
This is intended to be similar to the NAN class with the same name and same Handle-prefixed methods, to make porting from NAN to NAPI straightforward. I do prefer the On prefix myself. So I'd be OK with that if we think continuity from NAN is less important.
There was a problem hiding this comment.
See, I can't change it in NAN due to legacy vs value. I however urge you to improve on its design where possible, in favor of 1-to-1 compatibility. This particular change is nothing a simple find/replace cannot handle.
If someone is switching from NAN to NAPI, at the very least they already have to change the included header file. This change is not much harder than that.
Also, you were already going to do something different with the persistent method.
| } | ||
|
|
||
| inline void AsyncWorker::Queue() { | ||
| napi_async_set_data(_work, static_cast<void*>(this)); |
There was a problem hiding this comment.
If we don't want to take care of the node-0.10 support in our PR, then the napi_async_XXX api could be reverted back to direct uv calls.
There was a problem hiding this comment.
The typecast should be unnecessary, since void * is the top type of pointers.
There was a problem hiding this comment.
I still think that encapsulation of the uv calls here can make sense. Otherwise if uv changes then you won't get the abi stability. I know we won't wrap all of uv, but since this very commonly used I think it makes sense here.
- Rename some AsyncWorker methods as suggested - Create new ObjectReference and FunctionReference classes that make it easy to get/set properties and call functions via a reference with automatic handle scoping. Thse are both used by AsyncWorker, but are also likely to be useful in other scenarios. - Change many method parameters to accept NAPI primitives instead of NAPI classes. Since the classes are implicitly convertable to the primitives it doesn't affect passing class instances, but it makes it easier to interop with the NAPI C APIs that only use the primitives. - Move the index getters and setters from Array to Object, because Objects support indexed properties. This more closely matches the V8 object model anyway.
jasongin
changed the title
|
Added a big commit with a bunch of refactoring and improvements. I tested this version by updating the NAPI leveldown port to use this instead of |
|
@digitalinfinity For some reason I can't add you as a reviewer in this repo. But you might take a look if you have time. |
|
Looks good to me. 👍 |
|
OK, I'm not switching from napi to libuv async calls right now. If we decide to remove those APIs from NAPI then I'll update this code accordingly. |
AsyncWorkerclass that is an equivalent of the one defined innode_api_helpers.handnan.h, but done in the style of the other NAPI C++ wrappers.- Create new
ObjectReferenceandFunctionReferenceclasses that make it easy to get/set properties and call functions via a reference with automatic handle scoping. These are both used byAsyncWorker, but are also likely to be useful in other scenarios.ArraytoObject, because objects support indexed properties. This more closely matches the V8 object model anyway.After this, we should be able to delete
node_api_helpers.hand update any uses of the helper classes there to use the classes from here instead.