Skip to content
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

Merged
merged 6 commits into from
Feb 12, 2018

Conversation

gideongoodwin
Copy link
Contributor

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!

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);
Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@dcodeIO dcodeIO merged commit 462132f into protobufjs:master Feb 12, 2018
@dcodeIO
Copy link
Member

dcodeIO commented Feb 12, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants