V12 upgrade #150
V12 upgrade #150
Conversation
Venemo
left a comment
Venemo
left a comment
There was a problem hiding this comment.
Added a few comments to the code.
It overall looks good but please squash together the first few commits.
src/dbi.cpp
Outdated
| } | ||
|
|
||
|
|
||
| class PutWorker : public Nan::AsyncWorker { |
There was a problem hiding this comment.
Since you just added this in the previous commit, and you're deleting it right away, it would make sense to squash these two commits.
|
|
||
| isValid = true; | ||
| currentUint32Key = val->Uint32Value(); | ||
| uint32_t* uint32Key = new uint32_t; |
There was a problem hiding this comment.
Is the dynamic allocation really necessary here?
There was a problem hiding this comment.
I think so. We can't rely on stack allocation since this key is used outside the stack. And you had previously gotten around dynamic allocation by using a thread_local, and using a single slot that would be used sequentially in a single thread. But this key is now potentially going to be used across different threads, so a thread_local will no longer work, and I think we really do need to allocate memory for it until it is actually used (and we could have multiple keys allocated at once). Anyway, maybe I am missing something, and there is another way?
README.md
Outdated
|
|
||
| #### Asynchronous batched operations | ||
|
|
||
| You can batch together a set of operations to be processed asynchronously with `node-lmdb`. Committing multiple operations at once can improve performance, and performing a batch of operations and allowing the transaction to sync can be efficiently delegated to an asynchronous thread. The `batchBinary` method accepts an array of write operation requests, where each operation is an array: |
There was a problem hiding this comment.
It seems to me that this commit should also be squashed to the previous two. Since it just cleans up stuff that was introduced in there.
| Local<Value> options = info[1]; | ||
|
|
||
| if (!info[1]->IsNull() && !info[1]->IsUndefined() && info[1]->IsObject()) { | ||
| if (!info[1]->IsNull() && !info[1]->IsUndefined() && info[1]->IsObject() && !info[1]->IsFunction()) { |
There was a problem hiding this comment.
Since this one also fixes issues introduced by your PR, please squash this one too.
| bool isOpen = false; | ||
|
|
||
| EnvWrap *ew = Nan::ObjectWrap::Unwrap<EnvWrap>(info[0]->ToObject()); | ||
| EnvWrap *ew = Nan::ObjectWrap::Unwrap<EnvWrap>(Local<Object>::Cast(info[0])); |
There was a problem hiding this comment.
I see a lot of changes from ToSomething() to SomeThing::Cast - looks good but is this still compatible with older node versions?
There was a problem hiding this comment.
So I checked on this and tested on Node V8.x. These Cast calls seemed to compile and work fine. The one issue I did run into is is String::Write is incompatible between v12 and v8, so I added a conditional directive based on node version to get it to compile on both v12 and v8: https://github.com/Venemo/node-lmdb/pull/150/files#diff-718077d5c48933b84cfa39dd863e1461R281
I couldn't get my build tools to work before version 8, not sure how far back we are trying to support.
There was a problem hiding this comment.
FWIW, according to https://nodejs.org/en/about/releases/, I think version 6 has reached end of life and version 8 is the oldest maintained node version, so v8 seems like a reasonable cutoff.
|
I squashed all the async commits together, and left the v12 upgrade commit separate. |
2 participants
Fixes #149 , and addresses most deprecation warnings that can be fixes and support v11. However, this is branch is based on the async branch, because there is code in that branch that needs to be updated to new APIs as well.