Improve typecheck for svelte:component bind:this

[mvolfik]

This uses idea from #363, which however didn't work for HTMLElements and was therefore reverted in #402.

The resulting tsx looks like this:

<sveltecomponent this={ComponentType} {...__sveltets_1_empty((data[k] = __sveltets_1_instanceOf(ComponentType) || null))} />

As you can see, the ComponentType needs to be duplicated there¹, so this is waiting for Rich-Harris/magic-string#193 to be merged and included in a build.

Edit: why I'm doing this:

As shown in the output in the comment below, if I have a component with exported function () => string, it should be perfectly ok to assign this component to a variable expecting a function prop of type (a: number) => string | number (since it's a more concrete type, TS docs). However, the generated tsx currently requires type equality, so an error is raised. That's correct for bidirectional bindings like bind:value, but not for bind:this - since that only works bottom up (as far as I'm aware). So I fix that.

And the other thing I'm improving is the typecheck - there was none on <svelte:component bind:this={x} - this is expressed in the title, but I guess I should learn to document things more explicitely

Footnotes

1. well, it could be just moved and the this={} param removed, but perhaps we'll get some checking for that in the future as well ↩

[mvolfik]

Also, I'd like to ask if more end-to-end testing of this ecosystem (mainly svelte2tsx via svelte-check) was considered? Is there an issue open for this?

Because as we can see above, #363 looked like a good idea, but only after release it turned out to break some usage, which couldn't really be thought of when approving the changes in generated tsx.

For this PR, here's a test repo: https://github.com/mvolfik/fictional-potato (see steps in readme)

For reference:

v2.2.7

====================================
Loading svelte-check in workspace: /tmp/test
Getting Svelte diagnostics...

/tmp/test/src/App.svelte:51:23
Error: Argument of type 'C' is not assignable to parameter of type 'Simpler__SvelteComponent_'.
  Types of property 'action' are incompatible.
    Type '(a: number) => string | number' is not assignable to type '() => string'. (ts)
  <Exact bind:this={e} />
  <Simpler bind:this={s} />
  <Incorrect bind:this={i} />


/tmp/test/src/App.svelte:52:25
Error: Argument of type 'C' is not assignable to parameter of type 'Incorrect__SvelteComponent_'.
  The types returned by 'action(...)' are incompatible between these types.
    Type 'string | number' is not assignable to type 'string'.
      Type 'number' is not assignable to type 'string'. (ts)
  <Simpler bind:this={s} />
  <Incorrect bind:this={i} />



====================================
svelte-check found 2 errors, 0 warnings, and 0 hints

This PR:

====================================
Loading svelte-check in workspace: /tmp/test
Getting Svelte diagnostics...

/tmp/test/src/App.svelte:34:27
Error: Argument of type 'boolean | typeof Exact__SvelteComponent_ | typeof Incorrect__SvelteComponent_' is not assignable to parameter of type 'AConstructorTypeOf<Exact__SvelteComponent_, any[]>'.
  Type 'boolean' is not assignable to type 'AConstructorTypeOf<Exact__SvelteComponent_, any[]>'. (ts)
{#each [{ k: "simpler", v: Simpler }, { k: "exact", v: Exact }, { k: "incorrect", v: Incorrect }, { k: "weird", v: true }] as { k, v } (k)}
  <svelte:component this={show ? v : null} bind:this={data[k]} />
{/each}


/tmp/test/src/App.svelte:38:27
Error: Argument of type 'typeof Exact__SvelteComponent_ | typeof Incorrect__SvelteComponent_' is not assignable to parameter of type 'AConstructorTypeOf<Exact__SvelteComponent_, any[]>'.
  Type 'typeof Incorrect__SvelteComponent_' is not assignable to type 'AConstructorTypeOf<Exact__SvelteComponent_, any[]>'.
    Construct signature return types 'Incorrect__SvelteComponent_' and 'Exact__SvelteComponent_' are incompatible.
      The types of 'action' are incompatible between these types.
        Type '(a: number, b: number) => string' is not assignable to type '(a: number) => string | number'. (ts)
{#each [{ k: "simpler", v: Simpler }, { k: "exact", v: Exact }, { k: "incorrect", v: Incorrect }] as { k, v } (k)}
  <svelte:component this={show ? v : null} bind:this={data[k]} />
{/each}


/tmp/test/src/App.svelte:42:55
Error: Type 'Incorrect__SvelteComponent_' is not assignable to type 'C'.
  Type 'Incorrect__SvelteComponent_' is not assignable to type '{ action: (a: number) => string | number; }'.
    Types of property 'action' are incompatible.
      Type '(a: number, b: number) => string' is not assignable to type '(a: number) => string | number'. (ts)
{#each [{ k: "simpler", v: Simpler }, { k: "incorrect", v: Incorrect }] as { k, v } (k)}
  <svelte:component this={show ? v : null} bind:this={data[k]} />
{/each}


/tmp/test/src/App.svelte:52:25
Error: Type 'Incorrect__SvelteComponent_' is not assignable to type 'C'.
  Type 'Incorrect__SvelteComponent_' is not assignable to type '{ action: (a: number) => string | number; }'. (ts)
  <Simpler bind:this={s} />
  <Incorrect bind:this={i} />



====================================
svelte-check found 4 errors, 0 warnings, and 0 hints

[dummdidumm]

Didn't look at the code yet, but to answer some of your points:

• it's very unlikely the MagicString PR is getting merged anytime soon. I had some shortcomings with it, too, and thinking about monkeypatching it in here
• you can test diagnostics inside the language-server. There's a big file with all kinds of tests, add your case there. It's become messy so I'm thinking about restructuring these tests later

[dummdidumm]

I don't think that we need copy here, I don't see the need to have both mappings, the instanceOf(Component) thing does not need a reference to the original. Rename will work as it will add changes to the Svelte file, which does only have the this={Component} reference, which will trigger another Svelte->tsx transformation. Diagnostics will probably not span the whole element but that's acceptable to me (or could be handled specifically)

[mvolfik]

i'm sorry, i don't understand what you're saying. could you please explain it better/use other words?

[dummdidumm]

Yeah I expressed myself poorly here 😄 Maybe we turn it around: Could you explain why you think we need copy here?

[mvolfik]

my thought process:

• bind:this assigns a reference to the instance to the specified variable
• for normal components, the instance is just instanceof(ComponentClass)
• however, for svelte:component this={expression}, the expression could be anything, so it's instanceOf whatever is in the this={} clause
• I have no idea if there's some typechecking on the this={} - but there certainly could be something checking if it's [a component class or null], so I don't want to move it away, but instead copy

so if you were asking is why am I choosing to 'smart'-copy it with source mapping instead of just appending the string just like when it's a normal component, well, I have honestly no idea - it just looked like the right way to do it

[dummdidumm]

I'm not 100% sure on this change, it might break some people's code, and if it's not inside an if and not svelte:component with dynamic this, it's not true. Suppose you are in strict mode, then you could type the binding variable as let component: Component | undefined and this breaks for you (null !== undefined).

[mvolfik]

yeah, I'll remove it. people using this inside {#if} will hopefully be aware, and needless strictness over null|undefineds is annoying

[dummdidumm]

I merged a PR using yours as a base line. Thanks for bringing this up!