(feat) event typings

[dummdidumm]

#424

• now tries to detect event names from script/template if no typings given
• supports typed createEventDispatcher

To discuss: If the user types createEventDispatcher, do we want to be strict and throw type errors when it tries to listen to other events besides those defined in the typings and the bubbled events? Or do we still want to be loose and type all other events as CustomEvent<any>?

So for example if we have

<script lang="ts">
    import { createEventDispatcher } from 'svelte';

    const dispatch = createEventDispatcher<{ checked: boolean }>();

    // ...
</script>

<button on:click></button>

And then use it like

<ChildComponent on:unknownEvent={e => ...} />

Will this throw a type error the likes of "only click/checked are known" or is it allowed and the type is CustomEvent<any>?

What do we do if we decide it's an error and it's a false positive? Let's assume the user does import { clickDispatcher } from .. which is some kind of events dispatch mixin which makes it possible to listen to another event named anotherEvent. How will we make it possible for the user to relax the strictness without having to type ComponentEvents? Add a <script> attribute hasOtherEvents which optionally can be given a list of event names?

What do we do if we decide it resolves to CustomEvent<any> but the user wants strictness? Add a <script> attribute strictEvents?

@jasonlyu123 @pngwn

[pngwn]

This is an interesting one. The big challenge here of course is that events are essentially global, there is nothing to stop people from dispatching events manually as far as I am aware (via the CustomEvent interface) although I think it is an uncommon practice.

As a default, I'd err on the side of strictness since most event usage is simple and would be well covered by this feature. Type Errors would generally be warning users of actual problems rather than being a false positive.

As for how to configure it, I'm not sure. Is this the kind of thing that should be set globally or on a per file basis? Typescript has set some precedent here in that options are generally project level and opting out of specific behaviour is not really possible (unless you disable TS in its entirety for a line). Should we follow suit and make this a project setting or break with convention and go the eslint way of granular disable rules?

[dummdidumm]

I also tend to make strict the default. About "how to relax it": I tend towards a per-file-solution like this:

<script otherEvents>
   // ...
</script>

Adding the attribute otherEvents signals to us that there may be more events and the typings are relaxed. If the user wants a middle ground, we could also support this

<script otherEvents="name1,name2">
</script>

Which would mean "name1 and name2 are also allowed, but not others".

[pngwn]

I quite like that second option as a middle ground.

Do you think otherEvents or unknownEvents is the best wording?

[dummdidumm]

Good question. unknownEvents more accurately describes what this attribute is about, so I tend towards that.

[dummdidumm]

For reference: halfnelson and Rich are in favor or the "loose" option.

[pngwn]

We will have to fight to the death. There is no other solution.

[jasonlyu123]

Attribute for unknown events seems nice. But I think it would be nice if there's a global or per-project config to disable it, since having to add the attribute every time kind of destroy the purpose of a dispatcher mixin.

[dummdidumm]

So you also lean towards the "loose" option? What if everything is allowed and only if you add a strictEvents option it will be strict?
A global config could also be an option so everyone can choose what his preferred option is. How would the option look like? I'm hesitant to put it at the top level of the config, I'd rather have it inside something like svelteOptions or something like that so we can add more settings inside later if needed without breaking anyone else who might also use the config (Svite for example)

[jasonlyu123]

I am ok with strict by default. But for those with a lot of existing dispatch mixin or shared constant of events name, able to turn it off might be easier to gradually migrate to it.

[dummdidumm]

Mhm maybe loose should be the default then. It feels inconsistent to me that everything's loose, but as soon as you type createEventDispatcher, it's strict by default unless you set it otherwise.

So we would have something like

module.exports = {
  sveltePreprocess: ...,
  diagnosticOptions: {
     strictEvents: true,
  }

Or on a per-file basis

<script strictEvents unknownEvents="additionalEventName1, name2">
  //...
</script>

unknown events would help either way for autocompletion and in case of strictEvents it would mark those as valid events.

Your thoughts on this?

[dummdidumm]

I think I'm in the "loose-by-default" camp now. This means the PR can be merged as-is, and we can bikeshed on the options to make it more strict separately. I might even make a RFC with all the type specs that accumulated now to have some official "this is how you do it"-guide for others who might implement IDE support for Svelte.

[jasonlyu123]

Works pretty well. Have a simple suggestion on the string variables.
Also, there are two more situations that you can consider if we should support it.

1. import alias for createEventDispatcher

import {  createEventDispatcher as someAlias } from 'svelte'

and then track the alias name like dispatcherName
2. Maybe support something like this

createEventDispatcher()('abc')

[dummdidumm]

The alias is a good catch. The immediately-invoked dispatch is nothing we need to support I think - I tested that in the REPL and no event is fired (I guess it happens too soon), and you must invoke createEventDispatcher at the root of the component, not inside the function, so it technically isn't possible to dispatch an event like this.