-
Notifications
You must be signed in to change notification settings - Fork 137
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
Add missing methods to TypeScript definitions #766
Conversation
Size Change: 0 B Total Size: 3.25 MB ℹ️ View Unchanged
|
@@ -1058,7 +1059,9 @@ export class TransactionBuilder { | |||
options?: TransactionBuilder.TransactionBuilderOptions | |||
); | |||
addOperation(operation: xdr.Operation): this; | |||
addOperationAt(op: xdr.Operation, i: number): this; |
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.
is it getting to point where it's worthwhile to add ts
tests, index.d.test.ts
? how do we know addOperation
will work when used by a client, or is there some base validation done in build pipeline?
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.
There's JS unit tests for it but you're right that we should generally add more to the types/test.ts
file. Unfortunately said tests only fail when you, well, add to them, and if you're adding to them then you're adding to the index.d.ts
file anyway, so it wouldn't have prevented this slip-up (which came from an external contributor anyway).
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.
Added the typing tests in c5c5f9b!
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.
lgtm, and thanks for insight there on ts testing mechanics.
Self-explanatory, see diff.