fix(ejson): legacy mode now matches all compass test cases #347
Conversation
Rather than stringify -> parse just serializeDocument||Array directly like mongodb-extended-json
…ument compass compat
imlucas
changed the title
`eslint lib test --fix`
and remove date test case that appears to be invalid
There was a problem hiding this comment.
Just a note for other reviewers the changes to:
test/node/bson_test.js
test/node/map_tests.js
are just prettier formatting that didn't make it to master
I was concerned at first that the ability to override Object.prototype.toString would be an issue here:
Lines 175 to 177 in 6100b5c
but following the logic, objects that define their own
Symbol.toStringTag will just fall through serializeValue's checks and be passed into serializeDocument.
Just wanted to make notes of my findings LGTM! 🚀
| @@ -290,7 +290,7 @@ class Binary { | |||
| if (options.legacy) { | |||
| return { | |||
| $binary: base64String, | |||
| $type: subType.length === 1 ? '0' + subType : subType | |||
| $type: subType | |||
There was a problem hiding this comment.
What's motivating this change? Or rather, why was the padding needed in Durran's original implementation?
| @@ -150,6 +155,29 @@ const BSON_INT32_MAX = 0x7fffffff, | |||
| BSON_INT64_MAX = 0x7fffffffffffffff, | |||
| BSON_INT64_MIN = -0x8000000000000000; | |||
|
|
|||
| /** | |||
| * Serializes an object to an Extended JSON string, and reparse it as a JavaScript object. | |||
There was a problem hiding this comment.
Does this same docstring apply with the new implementation?
| return new BSONRegExp(doc.$regex, BSONRegExp.parseOptions(doc.$options)); | ||
| } else { | ||
| expr = new BSONRegExp( | ||
| doc.$regularExpression.pattern, |
There was a problem hiding this comment.
this seems to assume the existence of a $regularExpression property
pooooodles
added
the
tracked-in-jira
|
Thank you for submitting this PR! While our current development plan is at capacity, we will track this in Jira for future prioritization. |
This PR updates the legacy extended JSON compatibility introduced by #339 to pass the existing Compass EJSON legacy test suite mongodb-js/extended-json#91
Once this is merged, mongodb-js/extended-json#91 can proceed and
mongodb-extended-jsoncan be officially deprecated and archived.