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

useNewTransformation Feedback Thread #1352

Closed
dummdidumm opened this issue Jan 29, 2022 · 37 comments
Closed

useNewTransformation Feedback Thread #1352

dummdidumm opened this issue Jan 29, 2022 · 37 comments

Comments

@dummdidumm
Copy link
Member

dummdidumm commented Jan 29, 2022

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:

  • TS control flow easier to keep flowing
  • overall easier transformations of Svelte specific syntax like await, each etc.

You can try out the new transformation using

  • "Svelte for VS Code" version 105.11.1 and setting svelte.plugin.svelte.useNewTransformation to true
  • svelte-check version 2.4.0 with the --use-new-transformation flag.

Please provide feedback in here about any improvements or regressions.

@dummdidumm
Copy link
Member Author

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
Copy link

TomTomB commented Jan 30, 2022

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 pushed a commit to dummdidumm/language-tools that referenced this issue Jan 31, 2022
- move "xml:.." attributes into svg attributes interface
- remove quotes from attribute completions
- lowercase attributes when they are case insensitive

sveltejs#1352
dummdidumm added a commit that referenced this issue Jan 31, 2022
- move "xml:.." attributes into svg attributes interface
- remove quotes from attribute completions
- lowercase attributes when they are case insensitive

#1352
dummdidumm pushed a commit to dummdidumm/language-tools that referenced this issue Jan 31, 2022
Any attribute prefixed with data- is valid
sveltejs#1352
dummdidumm added a commit that referenced this issue Jan 31, 2022
Any attribute prefixed with data- is valid
#1352
dummdidumm pushed a commit to dummdidumm/language-tools that referenced this issue Jan 31, 2022
@dummdidumm
Copy link
Member Author

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

@swyxio
Copy link
Contributor

swyxio commented Feb 3, 2022

@dummdidumm found this while using the new setting:

image

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
Copy link
Member

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

dummdidumm pushed a commit to dummdidumm/language-tools that referenced this issue Feb 3, 2022
dummdidumm added a commit that referenced this issue Feb 3, 2022
dummdidumm pushed a commit to dummdidumm/language-tools that referenced this issue Feb 3, 2022
@dummdidumm
Copy link
Member Author

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

@ignatiusmb
Copy link
Member

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
Copy link
Member

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.

dummdidumm pushed a commit to dummdidumm/language-tools that referenced this issue Feb 4, 2022
sveltejs#1352
This makes use of the Svelte AST that is now returned from svelte2tsx (privately, do not use this elsewhere!) by checking if the AST node at the given position is of type text. This also helped refactor some other checks into something less hacky
@dummdidumm
Copy link
Member Author

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

@ZerdoX-x
Copy link
Contributor

ZerdoX-x commented Aug 30, 2022

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
Copy link
Member Author

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
Copy link
Contributor

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

@ZerdoX-x
Copy link
Contributor

ZerdoX-x commented Sep 9, 2022

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
Copy link
Member Author

dummdidumm commented Sep 13, 2022

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
Copy link

jrmoynihan commented Sep 14, 2022

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

dummdidumm added a commit to dummdidumm/language-tools that referenced this issue Sep 15, 2022
it's allowed to reassign them inside the loop
sveltejs#1352
dummdidumm added a commit that referenced this issue Sep 15, 2022
it's allowed to reassign them inside the loop
#1352
@samuelstroschein
Copy link

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
Copy link
Member Author

dummdidumm commented Sep 15, 2022

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
Copy link

aradalvand commented Sep 15, 2022

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
Copy link

@aradalvand You seem to have the same problem I had. See #1352 (comment)

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

@aradalvand
Copy link

aradalvand commented Sep 15, 2022

@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

This comment was marked as off-topic.

dummdidumm added a commit to dummdidumm/language-tools that referenced this issue Sep 16, 2022
for new transformation
sveltejs#1352

This adds additional types to
- incorporate custom element definitions into the new transformation
- override instead of merge type definitions
- allow easy SVGAttributes enhancement in the new trasnformation
dummdidumm added a commit that referenced this issue Sep 16, 2022
for new transformation
#1352

This adds additional types to
- incorporate custom element definitions into the new transformation
- override instead of merge type definitions
- allow easy SVGAttributes enhancement in the new trasnformation
@dummdidumm
Copy link
Member Author

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
Copy link

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
Copy link
Member Author

dummdidumm commented Sep 22, 2022

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

@dummdidumm
Copy link
Member Author

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

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

No branches or pull requests