Add LedgerHistory to Connection #1119
Conversation
FKSRipple
force-pushed
the
connection-cleanup-ledger
branch
from
9de66ef
to
fc52489
Compare
| } | ||
| }) | ||
| } | ||
|
|
||
| getLedgerVersion(): Promise<number> { | ||
| return this._whenReady(Promise.resolve(this._ledgerVersion!)) | ||
| async getLedgerVersion(): Promise<number> { |
There was a problem hiding this comment.
possible nit: missing an access modifier here. Is this on purpose?
There was a problem hiding this comment.
Which access modifier were you expecting? Since it's already a public method, we don't want to mark it private now and force a breaking change unnecessarily.
There was a problem hiding this comment.
I feel like my eslint config always yells at me to make access modifies explicit. I prefer to make things explicit, though if this library doesn't then I defer to the existing style.
There was a problem hiding this comment.
Oh like adding "public"? I've actually never used that before. Not against it, but yea lets hold off to a future PR if you're interested in making that change. This library has only used them in 1-2 places and I see our eslint passing clean.
Implements a change suggested in #1110.
Ledgerclass._whenReady(promise)->_waitForReady(). The old method appeared to not be working as expected: it was meant to run a promise after the connection was ready, but a promise would always start immediately at creation time. Instead we just remove the promise argument, and resolve once the connection is ready. Then, the caller can wait for this to resolve before moving on to start the thing that needed to wait.waitForReadywill set some ledger data, so that methods likegetBaseFee()will always return a number and never return null. It would be considered a bug if this promise was ever broken, but we could go one step further to throw an error OR add "null" as a possible return value so that the user had to handle this error case just in case. LMK your thoughts if you have any.