-
Notifications
You must be signed in to change notification settings - Fork 205
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
3.0.0: Type and formatting changes #853
Conversation
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.
Well, I'm no TS expert, but everything seems reasonable. I only have a couple questions to help me learn.
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.
Looks reasonable, suggested adding tests for some of the new logic in basemodel
/* eslint-disable no-param-reassign,no-return-assign,no-sequences */ | ||
return Object.keys(o).reduce((c, k) => ((c[k.toLowerCase()] = o[k]), c), {}); | ||
return Object.keys(o).reduce( |
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.
assume this is tested somewhere?
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 don't believe this is directly tested anywhere, but the changes here are only to the type annotations, which won't affect runtime behavior
|
||
if (format !== 'application/msgpack') { | ||
text = (body && new TextDecoder().decode(body)) || ''; | ||
} | ||
|
||
if (parseBody && format === 'application/json') { | ||
body = HTTPClient.parseJSON(text, res.status, jsonOptions); | ||
body = HTTPClient.parseJSON(text!, res.status, jsonOptions); |
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.
my understanding is that this should blow up if text was somehow null
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.
It's not possible for text
to be null/undefined at this point. If format === 'application/json'
, then the above if statement must have been executed, which assigns a string value to text
.
Typescript isn't quite smart enough to figure that out on its own, so I had to add the trailing !
, which is the non-null assertion operator. Does not affect runtime code at all
return BlockResponse.from_obj_for_encoding(encoding.decode(body)); | ||
} | ||
return undefined; | ||
return BlockResponse.from_obj_for_encoding( |
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.
assuming you've handled the empty case elsewhere?
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 actually removed the empty checking because I couldn't find any instance where it was used/required. If we get to this point, the response has a successful HTTP status code, so I'd expect the body to always be populated. That should be true of the block endpoint and the few others I changed in this PR at least
This is really a set of miscellaneous changes in preparation for later PR(s) which refactor the transaction class and change how the REST API decodes objects. In order to better isolate those important changes, I've made this.
For the most part, there should be no changes in behavior. The one exception to this is called out below.
Specifically, this is what's changed:
satisfies
operator in this case). This caused some formatting behavior to changetoString()
on the return value fromTransaction.txID()
(which is already a string)omitEmpty
argument toBaseModel.get_obj_for_encoding
. Prior to this, it was producing incorrect msgpack objects for encoding, since it wouldn't omit empty values.