-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Preserve comments when serializing/deserializing with toJSON and fromJSON. #983
Preserve comments when serializing/deserializing with toJSON and fromJSON. #983
Conversation
function MapField(name, id, keyType, type, options) { | ||
Field.call(this, name, id, type, options); | ||
function MapField(name, id, keyType, type, options, comment) { | ||
Field.call(this, name, id, type, undefined, undefined, options, comment); |
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.
Please confirm, this looks like an old bug calling base class constructor with wrong params.
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 believe omitting rule
and extend
and instead passing options
should be handled here. Might well be, though, that specifying undefined
explicitly is better optimizable by JS engines?
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.
Ah I didn't see the parameter shuffling in the Field constructor...I think it's clearer to explicitly pass undefined values for these, what do you think? Since options
can be undefined here as well, the duck typing in Field() gets a little nondeterministic if these values are omitted.
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.
Added additional parameter shuffling to handle the comment parameter.
Thanks! |
This version has an option controlling toJSON behavior. By default we'll skip comments when serializing to JSON, or they can be preserved by passing the new option. Thanks!