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

Destructuring syntax for computed properties #1069

Closed
Rich-Harris opened this issue Jan 2, 2018 · 7 comments
Closed

Destructuring syntax for computed properties #1069

Rich-Harris opened this issue Jan 2, 2018 · 7 comments

Comments

@Rich-Harris
Copy link
Member

I was about to respond to this HN comment explaining why it'd be impossible, but then I realised... it isn't!

computed: {
  hours: ({time}) => time.getHours(),
  minutes: ({time}) => time.getMinutes(),
  seconds: ({time}) => time.getSeconds()
}

As long as you do in fact use destructuring (i.e. not state => state.time.getHours()), Svelte could get all the information it needs to compile these computed properties. As a bonus, you could do things like this:

computed: {
  foo: ({bar: baz = qux}) => baz.toUpperCase()
}

(Aggressive destructuring results in unreadable garbage code, but hey, it's JS!)

It would be a breaking change, I guess (since we'd need to error on non-destructured arguments), but maybe it's a good one? Genuinely unsure. Thoughts welcome.

@tivac
Copy link
Contributor

tivac commented Jan 2, 2018

That would be a trivial-but-annoying change to make across our codebase. What's the benefit?

I don't even understand what the second version is doing, so maybe that's why I don't get the value proposition yet (beyond typing more characters).

@PaulBGD
Copy link
Member

PaulBGD commented Jan 4, 2018

I don't think it makes the code more readable and while it could give more options for destructing, I don't think it's worth a breaking change even for a major version.

@Conduitry
Copy link
Member

I voted thumbs down on this under the assumption that the old syntax would be removed. It doesn't seem obvious though that that would necessarily have to be the case. Couldn't both syntaxes be supported without resulting in ambiguity?

Even if both syntaxes were supported, I still don't see a lot of benefit to this. I think the existing syntax is perfectly fine.

@trbrc
Copy link
Contributor

trbrc commented Feb 20, 2018

Isn't it self-evident that it's preferable to stick to the standard language semantics than make up your own thing, unless the discrepancy affords some major benefit? Repurposing syntax like this is just weird. It's completely unexpected that the name of an argument affects what the function does, or that a particular function has to be a literal. I would also argue that it goes against the spirit of Svelte, from the guide:

Rather than reinventing the wheel, Svelte templates are built on foundations that have stood the test of time: HTML, CSS and JavaScript.

I think the discussion in this thread shows exactly why it is so problematic and urgent to fix, so not more Svelte users paint themselves into this corner. There's no benefit to having this odd thing around, and it's not even central to what Svelte is all about, but the longer it sticks around, the more painful it will be to remove it.

Here are some ideas for how to ease the transition:

  • Add a --legacy compiler flag that continues to parse argument names as dependencies
  • Create a codemod to automatically rewrite every existing computed property in the codebase

@TehShrike
Copy link
Member

The breaking change might be annoying, but this does seem like the better way to go. 👍

This would enable what I was talking about in chat a little while back (making the spread operator fully workable), and remove any need for #1303.

@Rich-Harris
Copy link
Member Author

I realised I promoted this in #1338 but didn't say anything in here, so:

I'm totally convinced that this is the right move. It solves real problems, and is less weird. The current dependency injection is something people often remark upon; it really grosses people out.

We can automatically upgrade components with svelte-upgrade.

@Rich-Harris
Copy link
Member Author

Ha, I just noticed that me and @trbrc both used the word weird in italics. Nuff said!

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

7 participants