js-tableland icon indicating copy to clipboard operation
js-tableland copied to clipboard

fix abort controller handling

Open sanderpick opened this issue 2 years ago • 8 comments

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

sanderpick avatar Jul 29 '23 00:07 sanderpick

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?

sanderpick avatar Aug 01 '23 14:08 sanderpick

Yeh those are D1 APIs, please don't change without ensuring they are backward compatible with 3rd party libs.

carsonfarmer avatar Aug 01 '23 15:08 carsonfarmer

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.

sanderpick avatar Aug 01 '23 16:08 sanderpick

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.

carsonfarmer avatar Aug 01 '23 16:08 carsonfarmer

Ooop, just noticed that all doesn't support colName anymore! So we could probably remove that to simplify the API?

carsonfarmer avatar Aug 01 '23 16:08 carsonfarmer

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 avatar Aug 01 '23 16:08 sanderpick

@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 avatar Aug 01 '23 16:08 joewagner

@joewagner Absolutely. Just wanted to get a first take on this. I'll try to get that done today.

sanderpick avatar Aug 01 '23 18:08 sanderpick