Integrate AbortController signals to storage fetch methods for APIs

[joshenlim]

What kind of change does this PR introduce?

• Integrate AbortController signals to storagefetch methods (GET, POST, PUT, REMOVE)
• Allow list endpoint to accept a signal parameter

Some context: An initial optimization I added on the storage UI when we were directly calling the endpoints via the fetch method was to add an AbortController, so that FE can cancel API calls if they have not completed.

This is particularly useful such that if the user opens a folder (which triggers a POST request to list the items in a bucket by a path), but then opens another folder immediately while the previous POST request is still in progress, the previous POST request will be cancelled and the latest POST request will be taken instead. With this, the UI won't have any flashing of the previously selected folder since the async POST requests are cancelled and not "queued".

I'm midway refactoring the storage UI to use the client library methods instead but would like to bring this behaviour into the client library as well as I think it might be really useful especially for calls that might be long running, and might possibly be useful in other parts of the library as well.

Example usage from the client

The client will maintain a context of the AbortController as such:

let controller = new AbortController()

And just pass an AbortSignal to the client library. Let's say the client has a function that invokes the list method from the client library, it can do something like this

const fetchBucketContents = async () => {
  // Discard any signal that might be present
  controller.abort() // Once the signal is aborted, the controller is "discarded"
  controller = new AbortController() // Hence we create a new one to replace

  const { data, error } = await supabase.storage.from('bucket').list('path', {}, controller.signal)
}

Note that calling controller.abort() just cancels the API call which it's signal was associated with. In that sense, managing the context of the signals on the client side is an important consideration. e.g If we have 2 functions, one that lists items of a bucket and the other is updating an item:

const fetchBucketContents = async () => {
  controller.abort()
  controller = new AbortController()
  const { data, error } = await supabase.storage.from('bucket').list(path, {}, controller.signal)
}

const updateItem = async() => {
  controller.abort()
  controller = new AbortController()
  const { data, error } = await supabase.storage.from('bucket').update(path, file, controller.signal)
}

Calling these functions successively in any order will cancel the first request which may be unintended.

Lemme know if anything can be improved!

Additional screenshots

Here's a screengrab to visualize the usage of abort controllers - I've deliberately throttled my network to make it more obvious

https://user-images.githubusercontent.com/19742402/114227479-a282c780-99a7-11eb-84f8-901b594b40a6.mov

[phamhieu]

Would you mind consider another solution

const fetchBucketContents = async () => {
  // Discard any signal that might be present
  controller.abort() // Once the signal is aborted, the controller is "discarded"
  controller = new AbortController() // Hence we create a new one to replace

  const { data, error } = await supabase.storage.from('bucket').list('path', {}, controller.signal)
  return {path, data}
}

// when you process fetchBucketContents response
// you can check path is the same as the selected folder to show list items
// if not you can show waiting indication

[phamhieu]

AbortController is available for browser fetch method only. So if we want to implement this, we need to use another polyfill version.

More detail can find here

[inian]

storage-js doesn't work in node already (cos we use form-data which isn't supported in node). We need to think of a way to add these without bloating the bundle size for browsers.

Looks like AbortController landed in Node 15. So adding a polyfill for this seems okay, since we can remove it over time.

One other suggestion is if we can have fetch parameters as an object.

await supabase.storage.from('bucket').list(path, {}, { signal: signal, timeout: 1000, fetch: fetch })

This way we can pass in other fetch related options overtime. One quick win would be that we can take in a custom fetch to use via this option where users want to use their own version of fetch (like in cloudflare workers here https://github.com/supabase/supabase/discussions/588)

[joshenlim]

Consider another solution

Hmm imo this might overcomplicate the logic within this feature - it'll involve an additional state to track the selected folder and an if else logic at the end of the fetchBucketContents API request. Trying to keep the code as simple as possible and figured AbortController can achieve a simpler, cleaner solution. Aborting calls also helps optimize things too.

"Another AbortController’s method is abort() which is able to cancel execution of a function. It means that if a server responds a browser knows to ignore that response and it won’t pass a callback to our callstack."

AbortController is available for browser fetch method only. So if we want to implement this, we need to use another polyfill version.

I don't think we need to install any new libraries in supabase-js though cause all we're passing is the AbortController's signal, not the controller itself? The context of the AbortController should be managed by the client, so if the client is a node client, then it'll need the node-abort-controller module, but the supabase-js library itself doesnt need the node-abort-controller

I've created a node repl to showcase this: https://replit.com/@joshenlim/StandardGentleHexagons#index.js

One other suggestion is if we can have fetch parameters as an object.

Oh yeap this looks good! Will update accordingly.

[inian]

the supabase-js library itself doesnt need the node-abort-controller

Oh of course...perfect!

[joshenlim]

Have updated to make the new input argument an object (FetchParameters) instead of just the signal - lemme know if anything can be improved!

[inian]

LGTM!

[joshenlim]

awesome thanks so much :) i'll merge the PR on monday evening if there's no other feedback - will wrap up the refactoring PR on the FE thereafter too!

[phamhieu]

Hmm imo this might overcomplicate the logic within this feature - it'll involve an additional state to track the selected folder and an if else logic at the end of the fetchBucketContents API request.

😁 yeah there are many ways to do thing. For me, it's easier to go wrong with AbortController

[phamhieu]

oh wow, we should separate storage into its own repo soon. If we don't do it now, maybe never.

In supabase-dart, there is a new PR to integrate storage api already...

[kiwicopple]

:tada: This PR is included in version 1.11.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: