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

fix: ensure createEventDispatcher works with types from generics #8872

Merged
merged 2 commits into from
Jun 28, 2023

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Jun 28, 2023

fixes #8860

This contains a small but unfortunately unavoidable breaking change: If you used never to type that the second parameter shouldn't be set (which the docs recommended for a short time), then you need to change that to null:

const dispatch = createEventDispatcher<{
-  noParameter: never;
+  noParameter: null;
}>();

Same for ActionReturn - use undefined instead of never:

- function action(el: HTMLElement): ActionReturn<never> {
+ function action(el: HTMLElement): ActionReturn<undefined> {
  // ..
}

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

fixes #8860

This contains a small but unfortunately unavoidable breaking change: If you used `never` to type that the second parameter shouldn't be set (which the docs recommended for a short time), then you need to change that to `null`
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

I will admit to having limited understanding of the explanation, but it sounds like this is necessary based on microsoft/TypeScript#54806, so lgtm

@dummdidumm
Copy link
Member Author

We'll need to do the same for the Action interface 😞 will get to it later

@GauBen
Copy link
Contributor

GauBen commented Jun 28, 2023

Hey! I observed a few interesting behaviors regarding the new createEventDispatcher type.

  1. This pull request slightly changes the behavior for null and undefined.

    // Setup
    const dispatch = createEventDispatcher<{
    	optional?: number;
    	nullable: number | null;
    }>();

    Before

    // Both are ok
    dispatch('optional');
    dispatch('nullable');
    
    // Both are ok
    dispatch('optional', undefined);
    dispatch('nullable', undefined);
    
    // Inconsistency
    dispatch('optional', null); // Error
    dispatch('nullable', null); // Ok

    After

    // All ok
    dispatch('optional');
    dispatch('nullable');
    dispatch('optional', undefined);
    dispatch('nullable', undefined);
    dispatch('optional', null);
    dispatch('nullable', null);
  2. As CustomEvent.detail defaults to null, the inferred types are slightly off when using undefined/?::

    const dispatch = createEventDispatcher<{ click: undefined }>();

    image

    image

    https://svelte.dev/repl/147bfc07869f4909b34f83951665155b?version=4.0.0

    Tracked in CustomEvent.detail is null when not defined microsoft/TypeScript-DOM-lib-generator#1585

I thought it was interesting to mention here, as tightening types might be a breaking change

@dummdidumm
Copy link
Member Author

dummdidumm commented Jun 28, 2023

Thanks for testing these out! The new behavior is actually more correct, as undefined and null will both result in null for event.detail

The last insight is actually unrelated, it's how the Svelte language server resolves these types - that hasn't changed, but should probably adjusted to be null for detail in that case.

@dummdidumm dummdidumm merged commit b0a3fa1 into master Jun 28, 2023
@dummdidumm dummdidumm deleted the event-dispatcher-fix branch June 28, 2023 21:56
@github-actions github-actions bot mentioned this pull request Jun 28, 2023
dummdidumm added a commit that referenced this pull request Jun 29, 2023
sebastinez added a commit to radicle-dev/radicle-interface that referenced this pull request Jul 7, 2023
We had to change the eventDispatcher for events that shouldn't have a
second parameter to `null` instead of `never`.

See: sveltejs/svelte#8872

Signed-off-by: Sebastian Martinez <me@sebastinez.dev>
rudolfs pushed a commit to radicle-dev/radicle-interface that referenced this pull request Jul 9, 2023
We had to change the eventDispatcher for events that shouldn't have a
second parameter to `null` instead of `never`.

See: sveltejs/svelte#8872

Signed-off-by: Sebastian Martinez <me@sebastinez.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

svelte-check/typescript detecting type mismatch where there shouldn't be one
3 participants