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

breaking: conditional ActionReturn type if Parameter is void #7442

Merged
19 changes: 12 additions & 7 deletions src/runtime/action/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
*
* Docs: https://svelte.dev/docs#template-syntax-element-directives-use-action
*/
export interface ActionReturn<Parameter = any> {
update?: (parameter: Parameter) => void;
interface ActionReturn<Parameter = void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value is not needed because everywhere a vaule gets passed

Suggested change
interface ActionReturn<Parameter = void> {
interface ActionReturn<Parameter> {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
interface ActionReturn<Parameter = void> {
export interface ActionReturn<Parameter = any>

not sure why this change slipped in (might have been an oversight by me), I think it's better to revert it to the previous version.

update?: void extends Parameter ? undefined : (parameter: Parameter) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use never instead of undefined

destroy?: () => void;
}

Expand All @@ -28,15 +28,20 @@ export interface ActionReturn<Parameter = any> {
* The following example defines an action that only works on `<div>` elements
* and optionally accepts a parameter which it has a default value for:
* ```ts
* export const myAction: Action<HTMLDivElement, { someProperty: boolean }> = (node, param = { someProperty: true }) => {
* export const myAction: Action<HTMLDivElement, undefined | { someProperty: boolean }> = (node, param = { someProperty: true }) => {
ignatiusmb marked this conversation as resolved.
Show resolved Hide resolved
* // ...
* }
* ```
* You can return an object with methods `update` and `destroy` from the function.
* If your action doesn't accept a parameter, type if as `Action<HTMLElement, void>`.
ignatiusmb marked this conversation as resolved.
Show resolved Hide resolved
*
* You can return an object with methods `update` (if the action accepts a parameter) and `destroy` from the function.
* See interface `ActionReturn` for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't export this interface anymore. Not sure if this line could be confusing to people reading it and then trying to import that type from svelte/action

*
* Docs: https://svelte.dev/docs#template-syntax-element-directives-use-action
*/
export interface Action<Element = HTMLElement, Parameter = any> {
<Node extends Element>(node: Node, parameter?: Parameter): void | ActionReturn<Parameter>;
}
export type Action<Element = HTMLElement, Parameter = any> =
void extends Parameter
? <Node extends Element>(node: Node) => void | ActionReturn<Parameter>
Copy link
Contributor

Choose a reason for hiding this comment

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

is <Node extends Element> really needed?
I would just type node as Element

: undefined extends Parameter
? <Node extends Element>(node: Node, parameter?: Parameter) => void | ActionReturn<Parameter>
: <Node extends Element>(node: Node, parameter: Parameter) => void | ActionReturn<Parameter>;
ignatiusmb marked this conversation as resolved.
Show resolved Hide resolved