-
Notifications
You must be signed in to change notification settings - Fork 253
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
NODE-2253: Support for v1 Extended JSON #339
Conversation
- Adds a legacy: true option to serialize and stringify - Adds tests for cases where v1 and v2 extjson differ.
474eb4d
to
4843287
Compare
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 with update to docs in README
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.
const base64String = Buffer.isBuffer(this.buffer) | ||
? this.buffer.toString('base64') | ||
: Buffer.from(this.buffer).toString('base64'); | ||
|
||
const subType = Number(this.sub_type).toString(16); | ||
if (options.legacy) { |
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 think we need to ensure options
exists, otherwise you'll have an error on trying to access legacy
on it. Recommend:
typeof options !== 'undefined' && options.legacy
- adding a
options = options || {}
above - make some helper function to check this, since its likely you are doing it many places
I realize this may be an existing issue prior to the changes introduced here, but this PR would necessarily introduce such a bug into every method now.
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.
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.
@imlucas added you as a collaborator
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.
Thank you. Pushed
legacy: true
option toserialize
,deserialize
,parse
, andstringify
.