js-tableland
js-tableland copied to clipboard
fix abort controller handling
Summary
Fixes abort controller handling which allows a user to abort and configure the polling timeout and interval for SDK methods that poll the gateway for a receipt.
Details
The core of this PR was adding the wiring to make the underlying polling handling aware of the user's abort controller. However, I found the API to be unintuitive and spent some time refactoring how a user goes about providing a abort-able controller with a custom timeout and polling interval. See comments below for details.
NOTE: This would be a breaking API change and therefore would require a major version bump. We probably could make the core fix while not breaking the API, but I don't think there's any need to be bashful about frequently releasing new major versions at this point. What do you guys think?
How it was tested
Updated tests to use the new API.
Checklist:
- [x] My code follows the style guidelines of this project
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [x] Any dependent changes have been merged and published in downstream modules
Related, after poking around in here, wouldn't it be cleaner to use a traditional options object on the statement api that included the colName for filtering and the "polling controller", ie, something like https://github.com/tablelandnetwork/js-tableland/commit/ef8f74f7acb51709d02f1c3dada2f69a21b55dd1 ?
Although, maybe this is part of the codebase that has to conform to D1?
Yeh those are D1 APIs, please don't change without ensuring they are backward compatible with 3rd party libs.
Yeh those are D1 APIs, please don't change without ensuring they are backward compatible with 3rd party libs.
K, do you have a ref to the API that you're conforming to here? I see a mention of D1 on the Database object, but nothing here.
Yeh, we should actually have a reference to its types. IIRC there was an issue a while back where we couldn't just do "implements" or "extends" because the D1 types didn't actually match the response you would get in practice. But they come from here: https://github.com/cloudflare/workerd/blob/main/types/defines/d1.d.ts and we can probably use the types from @cloudflare/worker-types now? Note that differences to these types are fine as long as they are backward compatible.
Ooop, just noticed that all doesn't support colName anymore! So we could probably remove that to simplify the API?
Cool, thanks! I see colName here: https://github.com/cloudflare/workerd/blob/main/types/defines/d1.d.ts#L26. Is that what you're looking at? Maybe it's just on first now in their API? btw, I don't have a strong pref here, it just felt weird to type undefined in the all method to be able to use the custom timeout.
@sanderpick This looks like great improvements in general. I agree the current API is very confusing, and turns out broken.
I notice you're pointing this PR at main and I was hoping to stop using this repo this week. However, in order to do that we need to get all the repos aligned on linting, prettier, and tests. I don't think there will be much issue here since the monorepo standards are mostly all being taken from this repo, but can you point this PR at the joe/lint branch?
@joewagner Absolutely. Just wanted to get a first take on this. I'll try to get that done today.