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.

Sign up for GitHub

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

Jump to bottom

Add Task args as argument to Task callbacks #4605

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add Task args as argument to Task callbacks #4605

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80b752f
Add task args to task callbacks as argument
Snapstromegon Mar 30, 2024
e4dc900
Add changeset
Snapstromegon Mar 30, 2024
ecf8080
Resolve comments by @justinfagnani to rename _previousArgs to _latest…
Snapstromegon Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/nasty-tomatoes-pretend.md
View file
Open in desktop
@@ -0,0 +1,5 @@
---
'@lit/task': minor
---

Add Task args to StatusRenderer callbacks
32 changes: 20 additions & 12 deletions packages/task/src/task.ts
View file
Open in desktop
Expand Up @@ -37,11 +37,11 @@ export const initialState = Symbol();

export type TaskStatus = (typeof TaskStatus)[keyof typeof TaskStatus];

export type StatusRenderer<R> = {
initial?: () => unknown;
pending?: () => unknown;
complete?: (value: R) => unknown;
error?: (error: unknown) => unknown;
export type StatusRenderer<R, D extends ReadonlyArray<unknown>> = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backwards compatibility: in case someone is using this type directly in an annotation, I think we need a default value for D so it can be omitted:

Suggested change
export type StatusRenderer<R, D extends ReadonlyArray<unknown>> = {
export type StatusRenderer<R, D extends ReadonlyArray<unknown> = ReadonlyArray<unknown>> = {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We call the args type T elsewhere, so I'd either stick with that or give all the type parameters better names like Result and Args.

initial?: (args?: D) => unknown;
pending?: (args?: D) => unknown;
complete?: (value: R, args?: D) => unknown;
error?: (error: unknown, args?: D) => unknown;
};

export interface TaskConfig<T extends ReadonlyArray<unknown>, R> {
Expand Down Expand Up @@ -423,18 +423,26 @@ export class Task<
return this._error;
}

render<T extends StatusRenderer<R>>(renderer: T) {
render<S extends StatusRenderer<R, T>>(renderer: S) {
switch (this.status) {
case TaskStatus.INITIAL:
return renderer.initial?.() as MaybeReturnType<T['initial']>;
return renderer.initial?.(this._previousArgs) as MaybeReturnType<
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_previousArgs doesn't sound quite right anymore if we're using them for the current render. Maybe it should be named _latestArgs?

S['initial']
>;
case TaskStatus.PENDING:
return renderer.pending?.() as MaybeReturnType<T['pending']>;
case TaskStatus.COMPLETE:
return renderer.complete?.(this.value!) as MaybeReturnType<
T['complete']
return renderer.pending?.(this._previousArgs) as MaybeReturnType<
S['pending']
>;
case TaskStatus.COMPLETE:
return renderer.complete?.(
this.value!,
this._previousArgs
) as MaybeReturnType<S['complete']>;
case TaskStatus.ERROR:
return renderer.error?.(this.error) as MaybeReturnType<T['error']>;
return renderer.error?.(
this.error,
this._previousArgs
) as MaybeReturnType<S['error']>;
default:
throw new Error(`Unexpected status: ${this.status}`);
}
Expand Down