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

(feat) rewrite svelte2tsx #1237

Merged
merged 104 commits into from
Jan 27, 2022
Merged

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Nov 9, 2021

Goal: Get rid of a tsx-style transformation and simplify transformations along the way
#1077
#1256
#1149

Advantages:

  • no messing with projects who use JSX for some other reason and then have conflicting JSX definitions
  • TS control flow easier to keep flowing
  • overall easier transformations of Svelte specific syntax like await, each etc.
  • better type inference in some cases, for example around svelte:component

This includes:

  • rewriting the html2jsx part of svelte2tsx
  • adjusting the language server to the new transformation
  • adding a toggle to language-server and vs code extension to turn on the new transformation. Default "off" for now, with the plan to switch it to "on" and then removing the old transformation altogether
  • ensuring tests run with both new and old transformation
  • adjusting the TypeScript plugin. It uses the new transformation already now since it's not easily possible to add an option (on/off). Should be low-impact since TS doesn't need to know much about the contents of a Svelte file, only its public API

Notes to self:

  • right now #each foo as foo and #await foo then foo are both valid Svelte code but the transformation is invalid ("variable used before declaration"). Solution could be to move the first foo into a different block like {const $$foo = foo; { const foo = await $$foo; } }. UPDATE this is handled now
  • <Component validPropWrongType1={true} invalidProp1={true} /> will only show an error for validPropWrongType1 and for invalidProp1 only after the first error is fixed. It will also underline validPropWrongType1 and not true. This is a hit I'm okay to take.

Simon Holthausen added 30 commits November 9, 2021 11:44
Goal: Get rid of a tsx-style transformation and simplify transformations along the way
sveltejs#1077
…ingsNamespace setting (better name for that? good API yet?)
…e, because of hover etc: This will make map deleted characters to the whitespace
Copy link
Member

@jasonlyu123 jasonlyu123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Besides those listed in the review comment. I also found some other problems:

  1. this would fail to transform because of magic string edits overlap
<script> 
	import { readable } from "svelte/store"; 
	const store = readable(11); 
</script>

<div {...$store}></div> 
  1. style directive and const tag doesn’t seem to have an expectedv2.json
  2. if you rename component on:some-event it would only show the first character of the event name in the input.

*p.s I deleted two outdated comments made a while ago. Can't delete it before I submit XD.

@dummdidumm
Copy link
Member Author

dummdidumm commented Jan 20, 2022

Thanks! I adressed your review comments. I also noticed that I should tweak Document Symbols a little, and that we need to update the TS plugin as well. Since there's no way to tell the TS plugin to either use the new or old version I decided to switch on the new version for the TS plugin by default. Need to test this more though.

I also added the possibility to add a namespace to the tsconfig for targets like SvelteNative like we discussed in Discord.

@jasonlyu123 in your opinion, will this new transformation enable us to enhance svelte2tsx with new transformations more easily? Is the transformation code easier or worse to read now in your opion?

@jasonlyu123
Copy link
Member

jasonlyu123 commented Jan 20, 2022

I think It's more focused on the core transformation. Overall should be easier to read.

On another note. I forgot to mention a problem yesterday. I found that the props transformation moves the props value now. So this causes the trailing property access to be removed under emitOnTemplateError. It's there because of the special preprocess before svelte parse.

For example:

<Component props={obj.} />

But this can also be done later. I'm thinking if we can detect this trailing property access while transforming. It should also be applied to the if block and await block so that the completion is right.

Edit: I tried and it seems possible. I think I'll have a PR for both v1 and v2 later once this is mered.

Another thing I found it's worth noting for users to migrate in the future is: Because we're now transforming it into direct component class instantiate. If you have a component typedef like this

export class Component {
  $$prop_def: SomePropsType
}

You can change this to use the SvelteComponentTyped class exported by svelte

export class Component extends SvelteComponentTyped<SomeProps> {}

if you're extending a component class that has different props types. Be sure to also override the constructor type.

@dummdidumm dummdidumm marked this pull request as ready for review January 22, 2022 09:46
@halfnelson
Copy link
Contributor

This looks great. I appreciate the work you did to guard it behind a feature flag 👍 which should allow us keen users to get our hands on it early and help work out any kinks.

@dummdidumm dummdidumm merged commit cad755f into sveltejs:master Jan 27, 2022
@dummdidumm dummdidumm deleted the svelte2tsx-rewrite branch January 27, 2022 14:42
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

Successfully merging this pull request may close these issues.

3 participants