Handling refreshes when callback may be called either synchronously or asynchronously?

[GormanFletcher]

During my component set-up I subscribe to data using a GraphQL library (urql) that caches responses. If the result is in the cache, the library calls my callback synchronously. If it has to hit the network, the callback is called asynchronously.

I store the query result in a variable and trigger a component refresh each time the callback is called. But since the callback can be called synchronously, this means sometimes this.refresh() gets called during the first render, resulting in the Component is already executing error. I can fix the synchronous render error with this.schedule(() => this.refresh()), but if the callback is called asynchronously, the scheduled function never fires.

What's the recommended way to handle this? For this case I can use setTimeout, but I've found refreshing to be difficult to get right in general - it often takes me some fiddling before my components update correctly. Keeping track of whether I need an immediate/scheduled/setTimeout refresh gets confusing. I'd like if Crank could automatically "do the right thing": if I tell it to refresh, then refresh again, no matter what state the component is in right now.

Does that make sense? Or is there a better way I should be going about this?

Edit: I'm not sure how this notion fits in with components where yields aren't in a loop, but instead do a sequence of meaningfully different things.

[brainkim]

resulting in the Component is already executing error

Technically, this is not a runtime error but a console “error.” Refreshes which are detected to occur while components are already executing are turned into no-ops, because if we just re-executed the component, we’d likely end up with an infinite loop. At the same time I was seeing people write refresh calls directly in the main function anyways, so I added this console error to discourage the practice.

I admit that I didn’t really think about the use-case of a zalgo-unleashing callback, but I can’t say that I care too much that a warning is emitted when you have an API whose callbacks run synchronously or asynchronously. Essentially, what’s happening is that a cache hit results in your callbacks being called synchronously, while a cache miss results in your callbacks being called asynchronously. For a callback API, doing both from the same function is incredibly confusing for several reasons as the article describes.

Ultimately, I think URQL designers thought they needed to update synchronously because of some perceived requirements, where it was unacceptable for a cache hit to produce a loading render. The thing is, successive DOM renderings which take place in a microtask loop are visually imperceptible, so I’m not sure why this is a requirement for GraphQL libraries at all. As an aside, requiring caches to return cached data synchronously severely limits the sort of backing storage and transport abstractions you can use.

In short, if you wrap the API so that the callback always fires after a microtask, you will both avoid the warning from Crank, and your users will be none the wiser. I think wrapping the URQL API so it doesn’t fire synchronously is probably a good idea.

Edit: I'm not sure how this notion fits in with components where yields aren't in a loop, but instead do a sequence of meaningfully different things.

Care to expand on this? If you have a component which doesn’t yield in a loop, I’d imagine you’d need an incredible amount precision around when the component is actually executed.

[GormanFletcher]

I think wrapping the URQL API so it doesn’t fire synchronously is probably a good idea.

I'll do this. I agree that enforcing consistent behavior from URQL is valuable.

More broadly than this example, having to consider what state a component could be in when I call this.refresh() has been a recurring pain point as I ramp up on Crank. Maybe it's something I'll build a better intuition for as I use Crank more? But in this early stage, I find myself frequently saying, "oh, this component didn't update. Is my state stale, or did I schedule when I shouldn't have?". I wind up adding or removing this.schedule(() => this.refresh()) more by trial and error than by methodical understanding of the component's needs (I understand the mechanisms, I'm just bad at thinking to apply the schedule before I see the bugs, or I don't know a priori whether some other third party library does synchronous callbacks).

I'd like to understand where this pain point falls in the balance between "tools should take reasonable steps to prevent easy mistakes" and "you should actually know how to use your tools". If the current behavior is driven by avoiding infinite loops, perhaps a solution could look like Crank taking a page from React and using an internal refresh counter to throw an error if it detects an infinite loop?

Care to expand on this?

I think I was thinking of examples from the docs of async generators that do multiple yields, but on reflection, that seems irrelevant here since async generators must run in loops and are expected to yield everything inside the loop without pausing on yields. So never mind :)

[brainkim]

having to consider what state a component could be in when I call this.refresh() has been a recurring pain point as I ramp up on Crank.

This is interesting to hear. Maybe we can add a boolean getter to contexts like isExecuting to help avoid the error? I’d have to think about this for a while to make sure there wasn’t a more elegant solution.

I find myself frequently saying, "oh, this component didn't update. Is my state stale, or did I schedule when I shouldn't have?"

I think part of this is just getting used to Crank and explicit refreshes. It will often be the case that you update a component’s local state, but forget to call refresh. The thing is, it’s a single-line change, and it means that Crank is compatible with roughly every data structure/state management solution/reactive library out there. I didn’t consider the zalgo callback situation, but I think in those cases, it’s better to wrap the API somehow so that the synchronous case doesn’t use callbacks. I have some broad sketches about how to do this, but I’d have to dive into URQL and its wonka abstraction to actually figure out the details.

I wind up adding or removing this.schedule(() => this.refresh()) more by trial and error

One thing I would say is, this.schedule(() => this.refresh()) is not meant to be used that often. It’s an idiom to advance a component immediately after render, so if you’re not sure the component is about to render it’s likely to just hang indefinitely. Typically, you would only use this.schedule(() => this.refresh()) right before yielding, in the main execution of the component. Scheduling a refresh in an independently executing callback feels like a can of worms. If you’re running into synchronous refresh errors, my suggestion will always be to defer refresh by a microtask.

I'd like to understand where this pain point falls in the balance between "tools should take reasonable steps to prevent easy mistakes" and "you should actually know how to use your tools". If the current behavior is driven by avoiding infinite loops, perhaps a solution could look like Crank taking a page from React and using an internal refresh counter to throw an error if it detects an infinite loop?

I like this framing. I think “preventing mistakes” represents the responsibilities of the library developer, and “learning how to use your tools” represents the responsibilities of the application developer, and the tricky thing is figuring out where the boundaries of these responsibilities are. Ultimately, I think there’s a lot of overlap; library developers should provide hints about development with the understanding that users have an incomplete picture of the inner workings of the library, and application developers should strive to make their understandings of libraries they use more complete.

In this specific situation, I think what you might be running into is that Crank is way more exacting with its executional semantics than React. A sync component is re-executed as many times as its parent renders it plus the amount of times refresh() is called. The edge case of a component refreshing while it is already executing is one which I’ve thought less about, but what I like about the current choices is that it doesn’t fudge the details of when refresh can or can’t be called. I like this because I think this is better for development in the long run.

React has gone the opposite direction, saying that you shouldn’t care about when or how many times the render function is called. I agree with this principle somewhat, but I think the best way for developers to gain an understanding of their libraries is to understand how they execute. The solution of an internal refresh counter logging an error if it “detects” an infinite loop, which from my personal experience is if the component is rerendered 3-4 times synchronously, hints at this tendency towards inexactitude, almost like they A/B tested the warning against live code to see how many renders they had to tolerate before users complained.

I dunno. I’m very hesitant to add an execution counter, but we could try it perhaps.

[GormanFletcher]

Maybe we can add a boolean getter to contexts like isExecuting to help avoid the error?

I thought about requesting that, but so far I've only thought of using it for cases where I'd decide whether to do a scheduled refresh. If that's its only use, it feels better to ask Crank to make the decision for me.

I’d have to dive into URQL and its wonka abstraction to actually figure out the details.

Right now I've updated my little wrapper around urql/wonka's subscribe function to enqueue my subscribe callback with queueMicrotask(). For one-off queries I think .toPromise()/await would suffice.

I think part of this is just getting used to Crank and explicit refreshes.

This is an important part of what makes Crank appealing to me. It's a minor hassle, but React makes components jump through big hoops to keep its deathgrip on your state, and I think manual refreshes are a reasonable cost to pay to get away from that.

this.schedule(() => this.refresh()) is not meant to be used that often
Scheduling a refresh in an independently executing callback feels like a can of worms.
I dunno. I’m very hesitant to add an execution counter, but we could try it perhaps.

It sounds like the answer is for me to get better at Crank and see if the problem fixes itself, and to leave the behavior as-is for now. I'll keep an eye out for whether I can nail down any insights worth submitting a documentation PR for.

[brainkim]

Right now I've updated my little wrapper around urql/wonka's subscribe function to enqueue my subscribe callback with queueMicrotask(). For one-off queries I think .toPromise()/await would suffice.

That’s probably how I’d do it! Let me know if you publish a wrapper library and want me to take a look. Alternatively, if you’d like my feedback on any code, I’m also happy to do that. I actually work on Apollo Client as a day job right now so I’m pretty actively comparing all the clients.