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
Add Task args as argument to Task callbacks #4605
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: ecf8080 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Tachometer Benchmark ResultsSummaryA summary of the benchmark results will show here once they finish. ResultsThe full results of your benchmarks will show here once they finish. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small required fix to keep this a backwards compatible change.
packages/task/src/task.ts
Outdated
pending?: () => unknown; | ||
complete?: (value: R) => unknown; | ||
error?: (error: unknown) => unknown; | ||
export type StatusRenderer<R, D extends ReadonlyArray<unknown>> = { |
There was a problem hiding this comment.
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:
export type StatusRenderer<R, D extends ReadonlyArray<unknown>> = { | |
export type StatusRenderer<R, D extends ReadonlyArray<unknown> = ReadonlyArray<unknown>> = { |
packages/task/src/task.ts
Outdated
pending?: () => unknown; | ||
complete?: (value: R) => unknown; | ||
error?: (error: unknown) => unknown; | ||
export type StatusRenderer<R, D extends ReadonlyArray<unknown>> = { |
There was a problem hiding this comment.
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
.
packages/task/src/task.ts
Outdated
switch (this.status) { | ||
case TaskStatus.INITIAL: | ||
return renderer.initial?.() as MaybeReturnType<T['initial']>; | ||
return renderer.initial?.(this._previousArgs) as MaybeReturnType< |
There was a problem hiding this comment.
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
?
Thanks @Snapstromegon! |
…Args and adapt generic types. Signed-off-by: Raphael Höser <raphael@hoeser.info>
Fixes #4561
This adds the args from the args function as an argument to the callbacks like mentioned here: #4561 (comment)
This is my first contribution to the projects, so I tried following CONTRIBUTING.md, but developing on Windows is hard in this project, since executing tests is not possible (other scripts only work in WSL and not natively in windows - at least for me).