fix(NODE-2944): Reintroduce bson-ext support #2823
Conversation
- Update benchmark imports - Add bson-ext test to CI
nbbeeken
requested review from
a team,
durran,
emadum and
dariakp
and removed request for
a team
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.
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.
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.
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.
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
| @@ -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.
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.
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.