Conversation
WD-33878
Demo
Jenkins 
demos.haus 
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
WD-33879
andogq
force-pushed
the
feat/data-loader
branch
from
d1a47dc to
d9de123
Compare
| } catch (error) { | ||
| // Ignore any errors thrown by the callback function. | ||
| return; | ||
| } |
There was a problem hiding this comment.
I wonder if this should use the logger to inform of unhandled errors in dev, otherwise this will just swallow the error.
| } | ||
|
|
||
| // This load is the latest one to complete successfully. | ||
| if (loadId >= latestSuccessfulLoad) { |
There was a problem hiding this comment.
Would we ever care about the response if it's not the very latest call? If not could it abort the other calls that are in progress?
Or another way to look at this might be: do we ever want to allow another call to be initiated if there's already a fetch in progress?
There was a problem hiding this comment.
We mightn't care about the data that's returned, which is why this check is here. This lets us temporarily use data from an old request whilst waiting for the result of a newer request.
The initiator of the load can also pass an AbortSignal to the load function, so they're capable of cancelling their own requests if something superseding it comes in. I don't think it makes much sense for the load to automatically be cancelled if a new one starts, as the thing that triggers a new load has full control over the previous load too.
See https://github.com/canonical/juju-dashboard/pull/2237/changes#diff-c68dd5398fe4e6af6d1b13b0273295f7979c5d6ffc4fbb1d17cc3af184f5ba92R103 for an example.
| function handleError(error: unknown): NonNullable<Source<unknown>["error"]> { | ||
| // Extract the message of the error. | ||
| let message = "An unknown error occurred"; | ||
| if (error instanceof Error) { |
There was a problem hiding this comment.
You could use this util here: https://github.com/canonical/juju-dashboard/blob/f3e515431e9167344c652cdcef076acf5c0eff5f/src/utils/toErrorString.ts
| }) | ||
| .finally(() => { | ||
| // Track the latest load to complete, favouring the latest one. | ||
| latestCompletedLoad = Math.max(loadId, latestCompletedLoad); |
There was a problem hiding this comment.
I think this needs to check if it was aborted as this will always run so an abort would cause this to update the id (if I've read this correctly).
There was a problem hiding this comment.
You've read it correctly, however we do want to track whenever a load finishes, whether it finishes successfully or not. That's why latestCompletedLoad is tracked in the finally, whilst the latestSuccessfulLoad is only tracked in the then branch.
3 participants
Done
Source<T>definition (WD-33878)createSource<T>to handle basic source house keeping (WD-33879)