solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
32.45k stars 926 forks source link

Resource refetch when source is falsy #1862

Open kixelated opened 1 year ago

kixelated commented 1 year ago

Describe the bug

I'm creating a resource that is triggered on user input and is quite fallible (requesting camera access). I want to refetch the resource when the user clicks.

My first inclination was to use the refetch method. However I don't want the resource to be fetched immediately. I figured that a fasly source would let me choose when to fetch, however this doesn't work. I think it's counter-intuitive that refetch is conditional instead of an override.

Your Example Website or App

https://playground.solidjs.com/anonymous/06e79e93-84e3-4977-9126-d4c0a1f0ec03

Steps to Reproduce the Bug or Issue

  1. const [value, { refetch }] = createResource(false, fetch)
  2. refetch()

Expected behavior

I was expecting the fetch method to be called.

Screenshots or Videos

No response

Platform

N/A

Additional context

The alternative is to create a separate loading signal. I've been doing that but it's very brittle and get out of state with resource.loading especially when exceptions are thrown into the mix.

In fact I tried to create an example of it in action but I failed a few times. Using a boolean loading signal doesn't work correctly because you need to set it back to false, which apparently sets the resource state back to unresolved??

Anyway here's what I settled on, but I think it would be much better if I could just call refetch: https://playground.solidjs.com/anonymous/0e878b05-ff06-4d4a-aaae-39e62274456e

ryansolid commented 1 year ago

I see. Yeah the reason for the design current was to prevent need to introduce more signals as the default. In a basic case resources refetch whenever the source changes. So having an on/off toggle would be yet another signal, so comboing it made sense for the most part. The concern here is as you said a boolean setting to true again isn't going to retrigger. It used to though.. It seems I memoized the source to have it execute under reactive context we prevented it from triggering again. This means that changing that behavior would have implications. Users would have to go back to memoizing themselves in these cases.

Calling refetch when false also has implications because in that case it is source-less.. Which is basically want you want here, but it means people could trigger it when in the wrong state. It is possible that override is on them. They could always check in the fetcher. So I am leaning towards this being the better solution. Sort of just separating the imperative API from the declarative one. Need to weigh how much we want to change resources at the moment with the 2.0 design underway, but this definitely should be considered.

kixelated commented 1 year ago

Yeah exactly.

When using a source, nothing happens when you call refetch with a falsy or duplicate value. IMO a more accurate name/type would be setSource: Setter<T>. But since you've already provided an Accessor<T | false | null | undefined>, you likely already have a Setter<T> or could make one pretty easily. This setSource would act as a temporary override the source next updates, which frankly seems like a confusing mix of declarative and imperative APIs.

When not using a source, refetch makes a lot more sense as an imperative API. It would be nice if there was a way to disable the initial fetch so you didn't need a declarative boolean signal, ex. when submitting a form.

My thought is that createResource should be split into either a fully imperative or fully declarative API. refetch is an imperative API and can only be available when no source is provided; performing a fetch with no validation.

kixelated commented 1 year ago

need to weigh how much we want to change resources at the moment with the 2.0 design underway

I ended up making a fork of createResource because:

  1. I had to create my own loading signal to handle refetching (this issue).
  2. I had to create my own error signal to catch retryable exceptions (fetcher wrapped with try/catch).

I think latest() almost works for the 2nd problem, but I want it to return undefined on error instead. Basically I want a Result<T, E> from Rust but I'm forced to work in Javascript. :)