Skip to content

Preserve comments when serializing/deserializing with toJSON and fromJSON.#983

Merged
dcodeIO merged 6 commits intoprotobufjs:masterfrom
gideongoodwin:gg-serialize-comments
Feb 12, 2018
Merged

Preserve comments when serializing/deserializing with toJSON and fromJSON.#983
dcodeIO merged 6 commits intoprotobufjs:masterfrom
gideongoodwin:gg-serialize-comments

Conversation

@gideongoodwin
Copy link
Copy Markdown
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!

Comment thread src/mapfield.js
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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