-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(NODE-2944): Reintroduce bson-ext support #2823
Conversation
- Update benchmark imports - Add bson-ext test to CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
test/unit/bson_import.test.js
Outdated
const { expect } = require('chai'); | ||
const BSON = require('../../src/bson'); | ||
|
||
describe('BSON Library Import', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we discussed organizing the tests here a little differently: using require.resolve instead of require, and splitting the tests into 2 describe blocks - one for bson-ext and one for js-bson, each of which should have one it block for 'should import the correct library' and another it block for 'should correctly roundtrip supported data types'; using helpers and/or a data table structure should help minimize code duplication
ignoreUndefined?: boolean; | ||
|
||
export interface BSONSerializeOptions | ||
extends Omit<SerializeOptions, 'index'>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we discovered that Serialize and Deserialize options end up being effectively the same here, so this code can be simplified accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked over this too quickly in our discussion, there is a difference and these keys below are the ones we desire from the underlying BSON library:
serialize options: checkKeys
serializeFunctions
ignoreUndefined
deserialize options: promoteLongs
promoteBuffers
promoteValues
fieldsAsRaw
I added a typescript test to cover this expectation.
test/unit/bson_import.test.js
Outdated
}); | ||
} | ||
|
||
describe('bson-ext should be imported if it exists', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny suggestion, can we make this describe just 'bson-ext'
and 'js-bson'
for the next one and then put the 'should be imported if it exists' into an it
block with the corresponding expects? I just think it'll read better in the spec printout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, did this 👍
@@ -1577,7 +1577,7 @@ describe('Insert', function () { | |||
var collection = db.collection('bson_types_insert_1'); | |||
|
|||
var document = { | |||
symbol: new BSONSymbol('abcdefghijkl'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BSONExt supports deserializing but not serializing this type (which is acceptable for deprecated types). So to make the bson-ext tests pass I've updated this to a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT :)
I've also updated the BSONSerializeOptions to use the options exported from our BSON library now, this pipes through the documentation from there so we don't have duplication.
While I'm here, (and before the major release) how do we feel about renaming
BSONSerializeOptions
->BSONOptions
?The truth is it is for both serialize and deserialize.