Converter: Fix 'bytes' field decoding into an empty array, now decoded into an empty Buffer #1020
Conversation
…d into an empty Buffer
|
Should fix #1012 |
|
@dcodeIO can you take a look on the fix? |
src/converter.js
Outdated
| @@ -245,7 +245,7 @@ converter.toObject = function toObject(mtype) { | |||
| ("}else") | |||
| ("d%s=o.longs===String?%j:%i", prop, field.typeDefault.toString(), field.typeDefault.toNumber()); | |||
| else if (field.bytes) gen | |||
| ("d%s=o.bytes===String?%j:%s", prop, String.fromCharCode.apply(String, field.typeDefault), "[" + Array.prototype.slice.call(field.typeDefault).join(",") + "]"); | |||
| ("d%s=o.bytes===String?%j:o.bytes===Array?%s:%s", prop, String.fromCharCode.apply(String, field.typeDefault), "[" + Array.prototype.slice.call(field.typeDefault).join(",") + "]", "util.newBuffer([])"); | |||
There was a problem hiding this comment.
Seems the default for buffer doesn't use the default value. What do you think of making an array all the time, similar to how it was before, but wrapping it with util.newBuffer conditionally for the !== Array case?
There was a problem hiding this comment.
Hmm, this'd still generate the same array literal two times, right?
There was a problem hiding this comment.
It looks like:
object.buf = options.bytes === String ? "" : options.bytes === Array ? [] : $util.newBuffer([]);
There was a problem hiding this comment.
Yeah, if the default value is empty. What's on my mind is something along the lines of:
if (options.bytes === String) object.buf = "...";
else {
object.buf = [...];
if (options.bytes !== Array) object.buf = $util.newBuffer(object.buf);
}There was a problem hiding this comment.
OK possible as well, pushed now.
The generated code is:
if (options.bytes === String)
object.buf = "";
else {
object.buf = [];
if (options.bytes !== Array)
object.buf = $util.newBuffer(object.buf);
}There was a problem hiding this comment.
@dcodeIO , is it better now?
…s when wrapping with 'util.newBuffer'
|
@dcodeIO , the PR is ready for a second review. |
|
Thanks for the heads up! Looking good to me now, except that it has some unnecessary whitespace in the |
src/converter.js
Outdated
| else if (field.bytes) { | ||
| var arrayDefault = "[" + Array.prototype.slice.call(field.typeDefault).join(",") + "]"; | ||
| gen | ||
| ("if(o.bytes===String) d%s=%j", prop, String.fromCharCode.apply(String, field.typeDefault)) |
There was a problem hiding this comment.
For example, can remove whitespace at ) d
src/converter.js
Outdated
| var arrayDefault = "[" + Array.prototype.slice.call(field.typeDefault).join(",") + "]"; | ||
| gen | ||
| ("if(o.bytes===String) d%s=%j", prop, String.fromCharCode.apply(String, field.typeDefault)) | ||
| ("else {") |
src/converter.js
Outdated
| ("if(o.bytes===String) d%s=%j", prop, String.fromCharCode.apply(String, field.typeDefault)) | ||
| ("else {") | ||
| ("d%s=%s", prop, arrayDefault) | ||
| ("if(o.bytes!==Array) d%s=util.newBuffer(d%s)", prop, prop) |
|
100%, whitespaces were removed |
|
@dcodeIO - I hope it's ready now |
|
Thank you! :) |
|
@dcodeIO , Hi, when the next release including this fix is expected? |
|
There are some build issues with node v10 currently that I haven't yet been able to investigate. |
|
@dcodeIO , when do you expect the next release to be issued? |
No description provided.