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

Issues/775 #779

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Issues/775 #779

wants to merge 17 commits into from

Conversation

pavelstudeny
Copy link

fixes #775

  • all unknown fields are deserialized into a single array __unknownFields
  • __unknownFields are serialized at the end
  • discardUnknownFields() remove the unknown fields from the deserialized data
  • a global option to ignore unknown fields is not present. if required, please suggest where to place it

* @param {Uint8Array} value bytes to add
* @returns {Writer} `this`
*/
Writer.prototype.rawBytes = function write_raw_bytes(value) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems that writeBytes (the one using util.Array) might be incorrect on older node versions, but it was probably never called until now

@jcgertig
Copy link

is this able to just handle encoding unkown Ids that are passed in like say

{
 SomeKnownKey: 1,
 9999: 'Some unknown id'
}

@jcgertig
Copy link

jcgertig commented Aug 15, 2018

Or do I have to put that id in the __unknownFields myself?

@pavelstudeny
Copy link
Author

@jcgertig it creates the __unknownFields automatically. Seeing this issue neither accepted nor rejected for more than a year, however, I'm wondering what's planned with this project now that Google provides built-in JavaScript support

@chakmingli
Copy link

If you fork and apply this change, it will be really helpful for many people.

@jeremyong
Copy link

jeremyong commented Jan 11, 2019

I looked at the tests and literally everything passed but there was an issue with CODECLIMATE_REPO_TOKEN.

This would be a really helpful merge, as this library isn't compliant with the protobuf specification without it.

@jeremyong
Copy link

@pavelstudeny Is there a reason you opted to make the unknown fields hidden?

@pavelstudeny
Copy link
Author

@jeremyong do you mean that the __unknownFields have enumerable set to false by "hidden"? Yes, since they are unknown, there is not much you can do with them in an enumeration.

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.

add unknown field support
4 participants