feat!(NODE-3472): convert to Node-API #137
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Address the TODOs asking for the state to be managed through smart pointers and as a proper class with destructors. This is helpful for the upcoming Node-API conversion, because the presence of C++ exceptions in Node-API (and the fact that Node-API support also comes with multithreading support) makes it important to use proper C++ RAII style in order to avoid memory leaks.
nbbeeken
approved these changes
Sep 22, 2021
mbroadst
reviewed
Sep 30, 2021
mbroadst
reviewed
Sep 30, 2021
mbroadst
reviewed
Sep 30, 2021
mbroadst
reviewed
Sep 30, 2021
mbroadst
reviewed
Sep 30, 2021
mbroadst
reviewed
Sep 30, 2021
|
Looks great, thanks for doing this! |
mbroadst
approved these changes
Sep 30, 2021
dariakp
approved these changes
Oct 1, 2021
There was a problem hiding this comment.
I think these changes are fine to go in as they are; two notes for completeness:
- I don't have any strong opinions on the empty options/string question, I think the justification for the behavior makes sense, so apart from better documentation I don't think there is anything for us to do.
- I'd like us to consider how we can accelerate the conversion to a promise-only API; the major release presents us with an opportunity to make that sort of a breaking change, but we can discuss that separately and it need not hold up these changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
mongosh integration patch: https://spruce.mongodb.com/version/614b2cf5e3c331087eee218b/tasks
chore: address TODO for managing state through smart pointers
Address the TODOs asking for the state to be managed through
smart pointers and as a proper class with destructors.
This is helpful for the upcoming Node-API conversion, because
the presence of C++ exceptions in Node-API (and the fact that
Node-API support also comes with multithreading support) makes
it important to use proper C++ RAII style in order to avoid
memory leaks.
feat!(NODE-3472): convert to Node-API