tsfn: add error checking on GetContext#583
Conversation
|
Lines 4113 to 4114 in e142a1e Will need to review if FATAL error is correct. If not, how to properly handle this error |
| Object::New(env), "Test", 1, 1, _ctx, | ||
| [this](Napi::Env env, Reference<Napi::Value> *ctx) { | ||
| _deferred->Resolve(env.Undefined()); | ||
| _deferred.reset(); |
There was a problem hiding this comment.
I don't think reset is necessary here as the unique ptr would call the deleter on destruct.
There was a problem hiding this comment.
👍 will update in later push, when master is updated with #583 since this PR conflicts
|
Hey @mhdawson / @gabrielschulhof , On the same basis of #566 (see #566 (comment)) , would this also be considered SemVer major? If so, should we put this in to the 2.0.0 release (the scope seems to be ever increasing...) In this case, "should not affect any code": If you were using This PR also directly conflicts with #580 because of the modifications of the test files, and as 580 is higher prio, I can't update this until that PR is merged. |
mhdawson
left a comment
mhdawson
left a comment
There was a problem hiding this comment.
LGTM after update based on comment in ThreadSafeFunction::New
|
@KevinEady do you know when you will be able to push the change to address the outstanding comment? |
|
Hi @mhdawson , just pushed! |
|
Landed as c881168 |
PR-URL: nodejs/node-addon-api#583 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: nodejs/node-addon-api#583 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: nodejs/node-addon-api#583 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: nodejs/node-addon-api#583 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
3 participants
Fixes: #581