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

(fix) destructuring inside $ #445

Merged
merged 9 commits into from
Aug 15, 2020

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Aug 11, 2020

#444

TODOs:

  • don't remove the ( ) around the expression if there is no transformation from $: ... to let ...
  • remove the ( ) in such a way that the inner transformations are preserved and not removed as well

@jasonlyu123
Copy link
Member

Also

let a = [1, 2, 3]
$: [ first ] = a

it would be nice if we can find the equivalent of this line in typescript
https://github.com/sveltejs/svelte/blob/f739b4772a4ad32c0fd337afb9d5594ebd776073/src/compiler/compile/Component.ts#L607

I looked into typescript.d.ts 'MemberExpression' seems to be the base interface of four other expressions in typescript

@dummdidumm
Copy link
Member Author

Good point, forgot about them. If you like you can take over, not going to work in this today anymore.

@tanhauhau
Copy link
Member

you can use periscopic#extract_names https://github.com/Rich-Harris/periscopic#extract_identifiers-and-extract_names to get all the identifier names within the assignment

@dummdidumm
Copy link
Member Author

Oh that's a nice way of getting it, but we are using the typescript AST here so we cannot use this.

@jasonlyu123
Copy link
Member

I think we could just recreate that function for typescript AST. Another option would be using typescript-eslint parser to transform AST to estree compliant but that seems a little bit too overkill

@jasonlyu123
Copy link
Member

When looking into the solution, I found another issue. Because we're transforming this:

$: a = b

to

let a = b

to infer type for a. So when one of the destructured identifiers is already defined but others don't, how do we transform it to do an assignment as well as declaration.

like this:

let a: number;
$: ({ a, c } = b);

@dummdidumm
Copy link
Member Author

This situation came to my mind, too, and my current solution for it is "that's such a corner case, just don't try to infer the type" 😄
So I would just transform it to

let a; // <- no type
$: ({a, c} = b);

@jasonlyu123
Copy link
Member

Oh! I just thought like "what about this situation you wrote?" 😂

@dummdidumm
Copy link
Member Author

Oooh you mean a different situation, I get it now. What if the user has defined one of them ... well yes, we somehow would have to add the other let. Maybe like this:

User:

let a: number;
$: ({a,b} = c);

Gets transformed to:

let a: number;
let b;
$: ({a,b} = c);

Sorry for the confusion 😄

@dummdidumm
Copy link
Member Author

So to summarize the cases:

No let x; by user

Input

$: ({a,b} = c);

Output:

let {a,b} = c;

At least one let x; defined by user

Input:

let a: number;
$: ({a,b} = c);

Output

let a: number;
let b; // <- we don't infer the type, too much work for such an edge case
$: ({a,b} = c);

Same goes for $: ([a, b] = c);

@jasonlyu123
Copy link
Member

I added a ts version of Rich's extract_identifiers in my branch
https://github.com/jasonlyu123/language-tools/tree/fix-destructuring-inside-himBHsj

@dummdidumm
Copy link
Member Author

Oh that's great 👍 I'll incorporate it into my changes.

@dummdidumm dummdidumm marked this pull request as ready for review August 14, 2020 09:44
@dummdidumm
Copy link
Member Author

Thanks a lot for that adapted method @jasonlyu123 ! Tests are passing now for both object and array destructuring. Could you take one more look at the code if everything is okay?

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.

Found some issue need some tweak

packages/svelte2tsx/src/nodes/ImplicitTopLevelNames.ts Outdated Show resolved Hide resolved
packages/svelte2tsx/src/svelte2tsx.ts Show resolved Hide resolved
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.

That's a clever workaround 👍

@dummdidumm dummdidumm merged commit 19f1f17 into sveltejs:master Aug 15, 2020
@dummdidumm dummdidumm deleted the fix-destructuring-inside-$ branch August 15, 2020 09:22
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.

None yet

3 participants