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

3.0.0: Type and formatting changes #853

Merged
merged 7 commits into from
Feb 5, 2024
Merged

Conversation

jasonpaulos
Copy link
Contributor

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:

  • Upgraded formatter to handle latest typescript syntax (the satisfies operator in this case). This caused some formatting behavior to change
  • Upgraded typescript and enabled strict type checking
  • Converted remaining non-typed unit tests to typescript
    • This and the bullet before massively help when performing a large refactor
  • Removed calls to toString() on the return value from Transaction.txID() (which is already a string)
  • This is the only thing that should result in a change in behavior: Added a new omitEmpty argument to BaseModel.get_obj_for_encoding. Prior to this, it was producing incorrect msgpack objects for encoding, since it wouldn't omit empty values.

@jasonpaulos jasonpaulos marked this pull request as ready for review January 23, 2024 21:23
Copy link

@jannotti jannotti left a 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.

tests/1.Mnemonics_test.ts Show resolved Hide resolved
tests/2.Encoding.ts Show resolved Hide resolved
Copy link
Contributor

@gmalouf gmalouf left a 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(
Copy link
Contributor

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?

Copy link
Contributor Author

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

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

Copy link
Contributor Author

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(
Copy link
Contributor

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?

Copy link
Contributor Author

@jasonpaulos jasonpaulos Feb 2, 2024

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

src/client/v2/basemodel.ts Show resolved Hide resolved
src/client/v2/basemodel.ts Show resolved Hide resolved
src/multisig.ts Show resolved Hide resolved
src/client/urlTokenBaseHTTPClient.ts Outdated Show resolved Hide resolved
@jasonpaulos jasonpaulos merged commit 2a25f7a into 3.0.0 Feb 5, 2024
2 checks passed
@jasonpaulos jasonpaulos deleted the type-and-formatting-changes branch February 5, 2024 15:21
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.

4 participants