Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

createResource support for array source shorthand (like on) #935

Closed
wants to merge 2 commits into from

Conversation

edemaine
Copy link
Contributor

@edemaine edemaine commented Apr 13, 2022

As discussed on Discord, this adds the option for an Array-of-Signals source argument for createResource.

createResource([signal1, signal2], ...) is shorthand for createResource(() => [signal1(), signal2()], ...). This feels very similar to on which accepts a single signal or an array of signals, and I've found myself desiring this when I want my resource to depend on multiple signals.

One current behavior this forbids: currently you can pass an array as the first argument and it will be treated as a static (not-called) source.

I'm not sure I got the types right, as TypeScript seems to be complaining about the tests I wrote. So if this gets approved functionality-wise, I'll have to summon the TypeScript experts to fix it. But I figured I'd wait until it's decided to be worthwhile.

Note that this uses the map Array-of-initial-size optimization from #934. If #934 is rejected, I can remove the initial size.

@otonashixav

This comment was marked as outdated.

@otonashixav
Copy link
Contributor

otonashixav commented May 1, 2022

Typing this is really hard, barring making the type more complicated and breaking backwards compatibility of the second generic, or doubling the number of overloads with a source (4 total to 6 total). See https://tsplay.dev/weXYBW.

Edit: Overloading is fine, if this makes it in 1.5 (since resources are being changed then I assume?) I'll provide the types then.


I'm going to make use of this comment to keep track of some other possible changes to the resource types in 1.5:

  • Disallow values for source that result in the fetcher never being called.
  • Fix instances of unnecessary generics (first two overloads, onHydrated).
  • Allow the types in refetching and refetch to be provided in the createResource call, defaulting to unknown.

@ryansolid
Copy link
Member

I'm still not keen on this as it feels unnecessary. And looking at it makes me wish I had simplified on not to have this form. There is no advantage to having these wrapped separately other than the small bit of typing it saves in the case of all Signals. It is convenient for things one level deep but it's also less obvious. It feels like potential future baggage for something that is arguably unneeded.

@edemaine
Copy link
Contributor Author

Yeah, fair enough. There's a question of whether you want arrays of signals to act like shorthand for a signal consistently across the interface, and you're leaning the other direction. FWIW, in addition to on, the renderer also has array conveniences, though there's a lot more useful behavior (diffing) there. Also somewhat related to the idea of ref arrays (see ryansolid/dom-expressions#149).

Another possibility would be to have an explicit higher-order helper function along these lines:

function combine(...funcs) {
  return funcs.map((func) => func());
}

Then you could write createResource(combine(signal1, signal2)) which is maybe slightly more convenient than createResource(() => [signal1(), signal2()]) (certainly a lot fewer parentheses). I'd still prefer the array convenience personally, but I could also define this helper function myself if I really want it (admittedly I probably won't unless it's in core).

One final idea is to enable calling fetcher with more arguments, which would be a more significant difference:

  1. What if source being an array had actual behavior differences, where createResource([signal1, signal2], fetcher) ended up calling fetcher(signal1(), signal2(), info)? (This feels a little asymmetric though; it's like an implicit Function.prototype.call.)
  2. What if createResource instead optionally took multiple sources, as in createResource(signal1, signal2, fetcher) which calls fetcher(signal1(), signal2(), info)? This feels nicely symmetric.

In the scenarios where I want to pass in multiple sources to a resource, I also want to immediately destructure the array, so this would be helpful in both the createResource call and the fetcher definition. Unfortunately these ideas are probably both difficult to type...?

@ryansolid
Copy link
Member

I'm going to close this one. It is more likely that createResource changes design than this. Adding more arguments to fetcher is not the direction I want to go especially considering things like server$ functions. Rather than this PR just rot away as the code gets further from it. I'm going to end it here. And we keep this a note for future design.

@ryansolid ryansolid closed this Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants