fix(NODE-6173): add rangeV2 feature flag toggle to Node.js bindings #809
Conversation
addaleax
requested review from
nbbeeken,
durran,
baileympearson and
dariakp
as code owners
| @@ -514,6 +514,10 @@ MongoCrypt::MongoCrypt(const CallbackInfo& info) | |||
| mongocrypt_setopt_bypass_query_analysis(_mongo_crypt.get()); | |||
| } | |||
|
|
|||
| if (options.Get("rangeV2").ToBoolean()) { | |||
There was a problem hiding this comment.
What happens if rangeV2 is provided? Does it return a false? If it can return null an additional check would be nice.
There was a problem hiding this comment.
I don't think I fully understand your question (probably a "not" missing somewhere?), but this is the equivalent of if (!!options.rangeV2) in JS 鈥撀爓e do a similar check right above this one for bypassQueryAnalysis. This type of check is fine for boolean values, including optional ones.
There was a problem hiding this comment.
Ah sorry, just found out that there was a not missing in my question. I was asking because we do options.get(flag).ToBoolean(), so if flag is not provided (is nullable in the ts definition) it would return null and fail with a NPE.
But if this is not the case, LGTM.
There was a problem hiding this comment.
Null and Undefined are represented in Node-API as distinct values, not a C/C++ nullptr, so that's all good, and .ToBoolean() corresponds directly to the JS standard ToBoolean() operation 馃檪
|
@nbbeeken Yeah, not ideal for this that |
See the ticket for motivation/why it would be convenient to have this earlier than the 'main' range V2 work 馃檪