sveltejs / language-tools

The Svelte Language Server, and official extensions which use it
MIT License
1.2k stars 195 forks source link

useNewTransformation Feedback Thread #1352

Closed dummdidumm closed 1 month ago

dummdidumm commented 2 years ago

We introduced a new transformation which powers the Svelte intellisense under the hood. The goal is to get rid of a tsx-style transformation and simplify transformations along the way.

Advantages for users:

Internal advantages:

You can try out the new transformation using

Please provide feedback in here about any improvements or regressions.

dummdidumm commented 2 years ago

Note for myself: We need to make the "no type definition found" or "type definition is wrong" error which we enhance for the old transformation here better for the new transformation and possibly also wrap the new Component call in a function which falls back to some defaults - don't know yet.

TomTomB commented 2 years ago

Hi, so far I've found 3 issues when using the new transformation.

Data attributes Using a data attribute (e.g. data-popper-arrow for popper.js) results in the following error

Argument of type '{ class: string; "data-popper-arrow": boolean; }' is not assignable to parameter of type 'HTMLProps<HTMLDivElement>'.
  Object literal may only specify known properties, and '"data-popper-arrow"' does not exist in type 'HTMLProps<HTMLDivElement>'

Some attributes got renamed This is a kind of breaking one. Before you could use something like xlink:href="someId" inside a svg. now it needs to be xlinkHref="someId". The same applys for xmlns:xlink and tabIndex (now tabindex).

Typing custom events from actions is a total loss Before, there was a workaround to type custom events dispatched via use:someAction.

// custom-events.d.ts
declare namespace svelte.JSX {
 interface HTMLAttributes<T> {
    'onclick-outside'?: (
      event: CustomEvent & {
        target: EventTarget & T;
      },
    ) => void;
  }
}
svelte component

<script>
  import { clickOutside } from 'somewhere';

  const onClickOutside = () => {
    console.log('foo')
  };
</script>

<div
  on:click-outside={onClickOutside}
/>

This now results in errors like

 The left-hand side of an arithmetic operation must be of type 'any', 'number', 'bigint' or an enum type.
',' expected 
 Argument expression expected
Declaration or statement expected
Cannot find name 'outside'
dummdidumm commented 2 years ago

Thank you @TomTomB for the valuable feedback! I adressed all issues in the latest version.

swyxio commented 2 years ago

@dummdidumm found this while using the new setting:

![image](https://user-images.githubusercontent.com/6764957/152308391-c34bf0d0-8c2a-4841-95d2-54436a9643ac.png)

it appears that it is not parsing multiline classnames properly? i can replicate whenever i just add an arbitrary newline in the middle of any classname, and it also goes away when i get rid of the newline.

ignatiusmb commented 2 years ago

<Nested.Component /> usage error seems to appear again

dummdidumm commented 2 years ago

Thanks @sw-yx and @ignatiusmb ! Both issues are hopefully fixed with the latest version.

ignatiusmb commented 2 years ago

Lightning fast as always @dummdidumm! Can confirm it's working again now. No more regressions detected in all the apps that uses the new transformation.

ignatiusmb commented 2 years ago

Oops, might've spoken too early. svelte-check indeed doesn't seem to report any more errors, but autocompletion are now a bit overzealous and treats everything as JavaScript, even outside <script> and { ... }, where emmet is supposed to take control.

To reproduce, try triggering autocompletion, or start writing HTML in markup. It works in an empty .svelte as well.

jasonlyu123 commented 2 years ago

Find one problem with Components having events in the typedef. Created #1375 for it.

dummdidumm commented 2 years ago

Thanks @ignatiusmb and @jasonlyu123 , both issues are hopefully fixed in the latest version.

ignatiusmb commented 2 years ago

Emmet is now working again! But seeing another regression with svelte-kit package generated components. Not sure if the library needs to re-generate the definitions again with the new version or it is actually incompatible, but reproduction below.

To reproduce, copy the .d.ts below that's been generated from an empty .svelte file and name it Missing.svelte.d.ts (don't even need to have the .svelte file itself), import it in another file (import Missing from '$lib/Missing.svelte';) and use it as usual.

/** @typedef {typeof __propDef.props}  MissingProps */
/** @typedef {typeof __propDef.events}  MissingEvents */
/** @typedef {typeof __propDef.slots}  MissingSlots */
export default class Missing extends SvelteComponentTyped<{}, {
    [evt: string]: CustomEvent<any>;
}, {
    default: {};
}> {
}
export type MissingProps = typeof __propDef.props;
export type MissingEvents = typeof __propDef.events;
export type MissingSlots = typeof __propDef.slots;
import { SvelteComponentTyped } from "svelte";
declare const __propDef: {
    props: {};
    events: {
        [evt: string]: CustomEvent<any>;
    };
    slots: {
        default: {};
    };
};
export {};

Error should show something along these lines, which is most likely from changes in #1375

image

rgossiaux commented 2 years ago

Just tried this on my own codebase and it caught an error that the old transformation does not catch (incorrectly using an attribute html-for= instead of for= due to bad conversion from React's htmlFor), so that was cool.

However I did get a new error when using <svelte:component> that I don't understand. This code throws an error:

<script lang="ts">
  import type { SvelteComponent } from "svelte";

  export let as: SvelteComponent;
</script>

<svelte:component this={as} />

as well as if you change the type from SvelteComponent to SvelteComponentTyped. What's the correct type here? I think the error message for this is also a bit confusing:

Error: Type definitions are missing for this Svelte Component. If you are using Svelte 3.31+, use SvelteComponentTyped to add a definition:
  import type { SvelteComponentTyped } from "svelte";
  class ComponentName extends SvelteComponentTyped<{propertyName: string;}> {}

Underlying error:
Argument of type 'SvelteComponentTyped<any, any, any>' is not assignable to parameter of type 'new (args: { target: any; props?: any; }) => Svelte2TsxComponent<any, any, any>'.
  Type 'SvelteComponentTyped<any, any, any>' provides no match for the signature 'new (args: { target: any; props?: any; }): Svelte2TsxComponent<any, any, any>'. (ts)

<svelte:component this={as} />

The text at the top of the error is probably helpful for some other case, but in this case it's just confusing.

(edit) I just realized I should have upgraded to the latest version (2.4.3) before commenting. That gives a couple errors that were not present in 2.4.1 in this component, which is a janky test helper I wrote before I discovered svelte-inline-compile: https://github.com/rgossiaux/svelte-headlessui/blob/master/src/lib/test-utils/TestRenderer.svelte

Error: Argument of type 'string' is not assignable to parameter of type 'never'. (ts)
        on:close={onClose}
        on:focus={onFocus}
        on:keydown={onKeydown}

They come from using on: on a <svelte:component> instance.

kevin-legion commented 2 years ago

I got only a couple of issues (one has been mentioned by @rgossiaux already):

  1. Dynamic components + svelte:component don't work as expected:
class MyComp extends SvelteComponentTyped<{
  name: string;
}> {}

// ...

const dynamicComp = ...;

// ...

<svelte:component this={dynamicComp} name="hello" />

Will fail with the following error:

Error: Type definitions are missing for this Svelte Component. If you are using Svelte 3.31+, use SvelteComponentTyped to add a definition:
  import type { SvelteComponentTyped } from "svelte";
  class ComponentName extends SvelteComponentTyped<{propertyName: string;}> {}

Underlying error:
Argument of type 'MyComp' is not assignable to parameter of type 'new (args: { target: any; props?: any; }) => Svelte2TsxComponent<any, any, any>'.
  Type 'SvelteComponentTyped<{ name: string; }, any, any>' provides no match for the signature 'new (args: { target: any; props?: any; }): Svelte2TsxComponent<any, any, any>'. (ts)
      <svelte:component this={dynamicComp} name="hello" />
  1. index in #each blocks always fails to compile:
{#each things as thing, index (index)}
  // ...
{/each}

Will fail as follows:

Error: Variable 'index' is used before being assigned. (ts)
{#each things as thing, index (index)}
  // ...

Other than that I'm really excited for the new transformation system, it catches much more type errors and is more reliable, amazing job 😃

dummdidumm commented 2 years ago

Thanks @ignatiusmb, @rgossiaux and @kevin-legion for the feedback!

Jojoshua commented 2 years ago

Just wanted to note that this feature has been working great. It's a game changer and gives our team comfort and enhanced productivity.

benbender commented 2 years ago

given a simple component with a $$Props-typing like:

  interface $$Props {
    size?: "s" | "m" | "l" | undefined;
  }

without the new tranformer, auto-completion in VSCode correctly only suggests "s" | "m" | "l" for the size-prop.

If I enable the new transformer, nearly every possible attribute is suggested. See:

Bildschirmfoto 2022-05-02 um 10 37 42
dummdidumm commented 2 years ago

Thanks for reporting @benbender, fixed in the latest version.

ZerdoX-x commented 2 years ago

Sharing feedback from the production project that uses sveltekit (not the biggest tho, ~50k lines of code, 40k of them is svelte files).

Before using new transformation we got 41 errors, 10 warnings, and 47 hints, after enabling new transformation svelte-check found 60 errors, 10 warnings, and 43 hints.

New errors we got and how we fixed them:

  1. mask-type:
    Error: Argument of type '{ ... "mask-type": string; ... }' is not assignable to parameter of type 'SVGProps<SVGMaskElement>'.
      Object literal may only specify known properties, and '"mask-type"' does not exist in type 'SVGProps<SVGMaskElement>'. (js)

    Solution: replace mask-type="alpha" with style="mask-type: alpha" Maybe related: https://github.com/gregberge/svgr/issues/336 Will also try to contact figma team about this "feature" as all svgs where imported from figma. I guess this property exists only in css scope, not as html attribute.

  2. lazy-loading for images
    Error: Argument of type '{ ... lazy: string; ... }' is not assignable to parameter of type 'HTMLProps<HTMLImageElement>'.
      Object literal may only specify known properties, and '"lazy"' does not exist in type 'HTMLProps<HTMLImageElement>'. (ts)

    Not sure how we got it in our codebase 😃 Thats good work for svelte-check. Replaced lazy="true" with loading="lazy"

After resolving new errors I've listed above, I get same errors count as before upgrading transformation. This upgrade also removed some unwanted falsy hints for me (#1580). Thank you for levelling up svelte-check!

dummdidumm commented 2 years ago

Thank you for the feedback! The mask-type is an interesting one, and after reading various threads on it I'm still a little confused whether or not this is (also) a valid attribute in the browsers (not in the final spec but still). In general, if you want to add attributes that are not part of our list of valid attributes but you know it's there (some custom attribute, or something not in the final html spec yet but the browser you cater for supports it), you can enhance the typings yourself. The process is described here for the old version and will be updated once the new transformation lands (the namespace/interface names to implement will be different then, but the then-old ones should still be applied).

In general, the new transformation may give errors for attributes with hyphens in their name. Previously, we used TSX under the hood for the intellisense, which allows unknown hyphened attributes. The new transformation uses TS under the hood, so it's more strict in that regard.

ZerdoX-x commented 2 years ago

I also found that we get Error: Argument of type 'string' is not assignable to parameter of type 'never'. (ts) error that @rgossiaux reported

dummdidumm commented 2 years ago

Could you provide a reproducible in a new issue? Can't reproduce this.

ZerdoX-x commented 2 years ago
image

for me it appears "randomly". it even appears in empty file with span only

it's an vscode error only. it doesn't appear in check tool

and I don't understand how do i find cause of this

dummdidumm commented 2 years ago

This sounds like a bug which is fixed in the latest version which was released a few hours ago. Try updating the extension and then reload VS Code.

ZerdoX-x commented 2 years ago

Hmm.. maybe yeah, it is fixed now. I will let you know if it will come back

ZerdoX-x commented 2 years ago

So I've made an issue for some another issue appeared for me after upgrading to new transformation. I've made it external because I thought it existed without using new transformation.

dummdidumm commented 1 year ago

The new transformation is now enabled by default

You can turn it off by setting svelte.plugin.svelte.useNewTransformation to false. The new transformation brings improved intellisense. If you find bugs, please report them. You can also provide feedback in here. After a testing phase, the old transformation will be removed an svelte-check version 3 with only the new transformation will be released. You can use the new transformation with svelte-check now already by running it with the flag --use-new-transformation true

jrmoynihan commented 1 year ago

Destructured object props are incorrectly identified as constants in {#each} blocks

Here's a simplified version of what I'm seeing:

CleanShot 2022-09-13 at 20 27 00@2x CleanShot 2022-09-13 at 20 27 21@2x

Seems to be correctly identified when I disable the new transformation setting:

CleanShot 2022-09-13 at 20 47 23@2x
samuelstroschein commented 1 year ago

With "svelte.plugin.svelte.useNewTransformation" set to false, defining custom web components works as follows:

// app.d.ts
declare namespace svelte.JSX {
    interface IntrinsicElements {
        "example-element": {
            name: number;
        };
    }
}
image

Problem

Setting useNewTransformations to true results in no type errors as if the namespace declaration is ignored.

Question

Is this a bug or am I supposed to declare custom web components in a different manner?

dummdidumm commented 1 year ago

You should be able to do the following:

// app.d.ts
declare namespace svelteHTML {
  interface IntrinsicElements: { .. }
}

That said, we probably should add backwards-compatibility for the typings. They exist for others, but not this case.

aradalvand commented 1 year ago

Hi, I have this in my app.d.ts:

type Stringable = string | { toString(): string };

declare namespace svelte.JSX {
  interface HTMLAttributes {
    class?: Stringable;
  }
  interface SVGAttributes {
    class?: Stringable;
  }
}

This is so that the "class" attribute accepts any object with that a toString method — I have a custom class builder that is a fluent API, and has a toString() method that actually produces the class list, I don't want to have to call toString() all the time because that happens automatically when the object is being set as the value of the attribute anyway, so I did this to get rid of the type errors; and it used to work prior to this change. Now it doesn't, and I'm getting type errors:

image

What should I do?

samuelstroschein commented 1 year ago

@aradalvand You seem to have the same problem I had. See https://github.com/sveltejs/language-tools/issues/1352#issuecomment-1248429583

Try changing declare namespace svelte.JSX to declare namespace svelteHTML.

aradalvand commented 1 year ago

@samuelstroschein I did that but sadly it makes no difference.

I think the problem in my case is that the language server concatenates the types with an &, for some reason, which makes no sense, I want to replace the type of that property/attribute entirely, just like how it previously used to behave.

aradalvand commented 1 year ago

@dummdidumm Should I create a separate issue for this? Thanks.

dummdidumm commented 1 year ago

Backwards compatibility should be better now, both these case should work without errors now, but you can also switch to the new svelteHTML namespace and it should work the same.

amartens181 commented 1 year ago

Backwards compatibility should be better now, both these case should work without errors now, but you can also switch to the new svelteHTML namespace and it should work the same.

I have Svelte for VS Code v106.1.0 and neither my old declare namespace svelte.JSX export interface HTMLProps<T> nor the new declare namespace svelteHTML worked with the new transformation.

dummdidumm commented 1 year ago

Could you please share a code snippet along with more details what doesn't work so we can reproduce this?

dummdidumm commented 1 year ago

svelte-check version 3 has been released which uses the new transformation