feat: improve types for createEventDispatcher

[ivanhofer]

This PR introduces a breaking change: the generic that can be passed to createEventDispatcher was improved, which means you can now specify which of the events have required arguments and which don't:

import { createEventDispatcher } from 'svelte';

// Svelte version 3:
const dispatch = createEventDispatcher<{
  optional: number | null;
  required: string;
  noArgument: null;
}>();

dispatch('optional');
dispatch('required'); // I can still omit the detail argument
dispatch('noArgument', 'surprise'); // I can still add a detail argument

// Svelte version 4 using TypeScript strict mode:
dispatch('optional');
dispatch('required'); // error, missing argument
dispatch('noArgument', 'surprise'); // error, cannot pass an argument

How to migrate

These changes might break your code if the generic is written in a way that assumes the old more lax typings.

• If you want to type an event as having no arguments, you now should type it as never
• If you want to type an event as being optional, you should now type it as YourType | null (this might catch some errors in your code on the usage side where event listeners assume that the type is always present)
• Required types function as before, but you now need to specify the argument (this might catch some errors in your code)

const dispatch = createEventDispatcher<{
-  optional: number;
+  optional: number | null;
  required: string;
-  noArgument: any;
+  noArgument: null;
}>();

- dispatch('required');
+ dispatch('required', 'string');

Note that you need to enable strict mode for TypeScript to take full advantage of the stricter types. Without strict mode, required arguments are still optional (this is a TypeScript limitation).

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

• Run the tests with npm test and lint the project with npm run lint

This PR adds some more sophisticated typings for the createEventDispatcher function.

Problem

The current implementation marks the detail parameter always as optional:

<script lang="ts">
   const dispatch = createEventDispatcher<{
      change: string
   }>()

   // expected call signature
   dispatch('change', 'string')

   // I don't have to pass something to `detail`
   dispatch('change')
</script>

And when accessing it in a parent component, It would only be marked as string but it actually would be string | undefined

<Component on:change={({ detail }) => /*  detail is of type `string` */} />

Solution

edited: it seems that detail is defaulting to null so I updated the example

This PR solves the issue in as follows.
If the type is marked:

• as never: no detail parameter required/allowed
• as some type e.g. string: the details parameter is required and must have the type of string
• as some type and null e.g. string | null: the details parameter is optional and can be of type string | null

Please see following examples:

const dispatch = createEventDispatcher<{
   loaded: never
   change: string
   valid: boolean
   optional: number | null
}>()

// @ts-expect-error
dispatch('some-event')

dispatch('loaded')
// @ts-expect-error
dispatch('loaded', 123)

// @ts-expect-error
dispatch('change')
dispatch('change', 'string')
// @ts-expect-error
dispatch('change', 123)
// @ts-expect-error
dispatch('change', undefined)

dispatch('valid', true)
// @ts-expect-error
dispatch('valid', 'string')

dispatch('optional')
dispatch('optional', 123)
dispatch('optional', null)
// @ts-expect-error
dispatch('optional', 'string')

The lines annotated with // @ts-expect-error are not valid and will throw a TypeScript error.

Note: some of those @ts-expect-error will only be valid if you have enabled the strictest type-checks for the file where you try this out.

Since this PR adds a few helper types to the lifecycle.ts file, I would suggest to move the types to a .d.ts file. Since there are no such types in the reop yet, I don't know how the Svelte-team want to handle this.

Regarding tests:
There are no types tests in the repo yet, so I propose to create a types folder in the test directory and paste the example above to a file inside that directory. The test command then would need to be extended with a tsc --noEmit command on this folder.

What do you think?

[ivanhofer]

I just saw that this repo is using TypeScript 3.7.5. In order for these tests to work, an upgrade to version 3.9.x is needed, which was released almost two years ago.
It would also be necessary to type some of the core files e.g. utils.ts which currently has no types at all.

Are the following points something the Svelte-Team would like to improve?

1. upgrade to Typescript 3.9.x
2. add types to some core files

pinging @dummdidumm since he probably knows the answers to my questions :)

[dummdidumm]

In #6770 I have the need, too, to bump the TS version. Adding more types is welcome, too. We just have to ensure we don't introduce new syntax in the public API that isn't supported in older versions of TS

[jasonlyu123]

Maybe we could try downlevel-dts if we want new typescript features in public API.

[ivanhofer]

In #6770 I have the need, too, to bump the TS version. Adding more types is welcome, too. We just have to ensure we don't introduce new syntax in the public API that isn't supported in older versions of TS

Thanks for your response. I'll try to add some more types that are compatible with 3.7.5.

[ivanhofer]

@dummdidumm I saw you are upgrading to version 4.0
I would only need version 3.9 for my PR but I assume I should also upgrade to version 4.0?

[ivanhofer]

@dummdidumm I have created a wrapper TODO type that marks some parts of the code where I'm unsure if an optionaly Type | undefined get's assigned to a Type. I don't know the code well enough to tell if the undefined case can happen or if there is a potential bug because of not handling the undefined case.

Since these changes will be really time-consuming, I'm asking if I should proceed with this PR?

This TODO type would actually be my proposal to allow to enable the strict option on the global level.
It could be a somehow fast way to get rid of TypeScript errors and at the same time mark it as a task for the future.
This would:

• requires no code changes (except for casting errors to TODO)
• "enforce" new contributions to think about stricter type-checks

I'm not sure if the Svelte-Team wants that.
I would help making strict happen in another PR.

[ivanhofer]

I now have split my changes so this PR just includes the types for createEventDispatcher and I have created a new Draft PR #7324 that contains the upgrade to TypeScript version 3.9.5, type definitions for some core functions and type tests for createEventDispatcher.

The changes made to the createEventDispatcher type are compatible with the current TypeScript version of this Repo (3.7).
Here is a link to an isolated example in to TypeScript Playground

Nevertheless this PR could introduce a breaking change since a new (totally valid) error-message could show up in some codebases.

[ivanhofer]

If you want to benefit from the stronger types for createEventDispatcher right now and don't want to wait until this get's merged, you can use following workaround:

You just have to create a createEventDispatcher.d.ts file inside your src folder with following content:

import 'svelte'

declare module 'svelte' {
   type UnionToIntersection<U> = (U extends any ? (k: U) => void : never) extends (
      k: infer I
   ) => void
      ? I
      : never

   type ExtractObjectValues<Object extends Record<any, any>> = Object[keyof Object]

   type ConstructDispatchFunction<
      EventMap extends Record<string, any>,
      EventKey extends keyof EventMap
   > = EventMap[EventKey] extends never | null
      ? (type: EventKey) => void
      : null extends EventMap[EventKey]
      ? (type: EventKey, detail?: EventMap[EventKey]) => void
      : (type: EventKey, detail: EventMap[EventKey]) => void

   type CreateDispatchFunctionMap<EventMap> = {
      [Key in keyof EventMap]: ConstructDispatchFunction<EventMap, Key>
   }

   type EventDispatcher<EventMap extends Record<string, any>> = UnionToIntersection<
      ExtractObjectValues<CreateDispatchFunctionMap<EventMap>>
   >

   /**
    * @example
    * ```ts
    * const dispatch = createEventDispatcher<
    * 	click: number // define the type of the event's `detail`
    * 	loaded: never // use `never` if the event should not contain any payload
    * 	'custom-event': string | null // use an union type and add `null` if the payload is optional
    * >()
    * ```
    */
   export declare function createEventDispatcher<
      EventMap extends Record<string, any> = any
   >(): EventDispatcher<EventMap>
}

[tanhauhau]

i wonder if we can introduce different type definitions using typeVersions?

https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#version-selection-with-typesversions

[ivanhofer]

Sounds promising.

Could be a bit painful to link everything well together.
From what I understand, we would have to remove the types from the function inside lifecycle.ts and create a pre_3_9.d.ts and a post_3.9.d.ts file with the corresponding type definitions.

What I could not find out from reading the docs is: can you use this to enhance single typedefinitions or do you have to include all typedefinitions in that specific path? Maybe I find some time to try this out next week.

[benmccann]

Per my comment on #7324 (comment), I think we can get that PR in soon, so it shouldn't be a blocker. Mind updating the merge conflict on this one?

[vercel]

@ivanhofer is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

[ivanhofer]

@benmccann done.

Tested it with follwing cases and it looks good!

[dummdidumm]

Thanks for bringing it up to date!
Strictly speaking this is a breaking change because people could rely on the optional behavior currently. So I think this needs to land in version 4. To safe this PR from needing another rebase we should probably merge this in the v4 branch right before all the other monorepo conversion stuff etc

[ivanhofer]

To safe this PR from needing another rebase we should probably merge this in the v4 branch right before all the other monorepo conversion stuff etc

Do you change the base branch? I can't see a v4 branch.

[dummdidumm]

There is none yet, details of the process are not fleshed out yet.

[dummdidumm]

I was able to simplify the types (and they also read better when using them now) taking the learnings from #7442. I added tests, but we need to check them manually for now since we need strict mode for checking them, and it pull in too much other files when doing that through the CLI which we would need to convert to strict types. For Svelte 5 this should be solved.

[opensas]

I'm giving a try to these new stricter types for createEventDispatcher and it seems that using never for no argument events is not working.

given:

const dispatch = createEventDispatcher<{
  dismiss: never;
}>();

dispatch('dismiss')

I get the following error:

Expected 2-3 arguments, but got 1.ts(2554)
⚠ Error(TS2554)  | Expected 2-3 arguments, but got 1.

const dispatch: EventDispatcher
<"dismiss">(type: "dismiss", parameter: never, options?: DispatchOptions | undefined) => boolean 

and using void, null or undefined (the example from the migration guide uses null instead of never, so I guess someone already noticed this issue with never) allows null and undefined to be passed as parameter to dispatch.

Should I create a different issue for this?

---

$ npx envinfo --npmPackages svelte,svelte-check,@sveltejs/kit,typescript,tslib,vite --binaries

  Binaries:
    Node: 16.20.1 - /usr/bin/node
    npm: 8.19.4 - /usr/bin/npm
  npmPackages:
    @sveltejs/kit: ^1.21.0 => 1.21.0 
    svelte: ^4.0.3 => 4.0.3 
    svelte-check: ^3.4.4 => 3.4.4 
    tslib: ^2.6.0 => 2.6.0 
    typescript: ^5.1.6 => 5.1.6 
    vite: ^4.3.9 => 4.3.

[dummdidumm]

I'm sorry, the migration instruction was a bit outdated, you should use null to specify that the event receives no argument. Updated.

[opensas]

thanks for the response, the problem with null is that is allows to pass null and undefined (it's just a minor thing, but it would be better if it could strictly check for no param at all being passed)

and with never:

btw, docs in the migration guide correctly tell us to use null

[dummdidumm]

Passing null or undefined and passing nothing all result in event.detail being null at runtime, so it may feel a bit incorrect but is actually correct. Furthermore, you may want to pass the third parameter options, at which point you need to be able to pass something to the second parameter.