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

Spread properties and special *state* property #195

Closed
Swatinem opened this issue Dec 14, 2016 · 28 comments
Closed

Spread properties and special *state* property #195

Swatinem opened this issue Dec 14, 2016 · 28 comments

Comments

@Swatinem
Copy link
Member

It would be nice to support spreading into component props and to have a special identifier to access the whole local component state.

Spread could look something like this:

{{#each rects as rect}}
  <Rect {{...rect}} />
  <!-- or -->
  <Rect ...{{rect}} />
  <!-- same as -->
  <Rect x="{{rect.x}}" y="{{rect.y}}" />
{{/each}}

And having a special identifier for the complete state object would make decorator components really awesome, like so:

<!-- RectDecorator.html -->
<Rect {{..._}} foo="bar" />
<!-- same as -->
<Rect x="{{x}}" y="{{y}}" foo="bar" />

<!-- main.html -->
<RectDecorator x="1" y="2" />
@PaulBGD
Copy link
Member

PaulBGD commented Dec 14, 2016

The ... for spreading is a JavaScript feature, maybe we should use a different indicator for spreading since Svelte templates aren't JavaScript.

@Swatinem
Copy link
Member Author

Hm, I thought of ... to be more in alignment with how react does things. But you are right. Maybe we can leave it off, and just make it <Rect {{rect}} />, so if a mustache is not inside an attribute, its considered a spread…

@Rich-Harris
Copy link
Member

Will close this now we have shorthand attributes (<Rect :x :y/> and so on).

@TehShrike
Copy link
Member

The lack of spread continues to be a big pain for me, adding lots of difficult-to-maintain cruft in my components.

Having to maintain a list of all possible attributes that I might ever need to pass through a component is causing me a lot of friction in my most composable components.

@Rich-Harris
Copy link
Member

@TehShrike PRs welcome 😀 (by which I mean I'm on vacation next week and slammed until then, so it's unlikely I'll be able to work on spread any time soon)

@TehShrike
Copy link
Member

No worries, I was just thinking that this issue should probably get necro'd back to open.

@mrkishi
Copy link
Member

mrkishi commented Jan 24, 2018

Since there was reluctance on #280, here are my two (and a few more) cents:

I think this feature would be awesome. And I'm not talking about passing several properties easily, but the component decoupling aspect of it. That's an important distinction.

Most of the times I felt the need, there was actually an easy solution in encapsulating the properties, ie. changing the component API. It may be tedious (if not ugly) sometimes, but it's an easy fix.

It gets really weird when you want to use a component's default data and you can't, because you don't know if you have data to override it. This is a problem I couldn't get around unless messing with Component.data() directly or being extremely verbose with {{#if}}{else}}s (and being statically tied by doing so).

(this explanation is not gonna be succinct, but I couldn't express this clearly, sorry!)

Let's start with a simple component MainComponent with two routes: List and Item. repl.

Let's move to <:Component> by defining a unified route component API consisting of only the title and data properties. This is slightly less ergonomic inside routes, but much more ergonomic for the router. repl.

That looks perfect, until you realize you've lost the ability to use the routes' data() automatically. repl.

When you have <Route :data/>, whatever your data is (even undefined with hasOwnProperty('data') === false), it'll override Route's default data. Now, this makes sense because we would want the same behavior as set({ data: undefined }). But now we have to do something like new MainComponent({ target, data: { route: Route, data: Route.data(), ... }) to get around that. Not pretty, especially when combined with the rest of your data needs...


When we're dealing with a large amount of routes with SSR, data prefetching and lazy loading route chunks, the router tends to be a decoupling point. We want to minimize specific route components knowledge in the router, because it becomes too verbose and gets in the way of our chunking process (think accepting components from different teams with no static dependency).

Svelte started with no decoupling anywhere, with everything available at compile-time. Then <:Component> introduced separation at the component level -- but they're still coupled at properties. The spread feature would fill that gap. I see it as an intentional separation as opposed to an accidental shot at static analysis. I'd go as far as suggest that the spread feature should only work in <:Component> as to reduce its usage as a properties bag shortcut everywhere (which could in fact hinder future optimizations).

This is what our example could look like with spread: repl. Note how we regain the routes' data flexibility while keeping data() useable.


If you're still open for PRs, I'll take a look into it -- but I may need some help with Svelte's internals. 😀

@PixievoltNo1
Copy link

To paraphrase my remarks from my own issue on this, there are some good use cases for this feature, such as a tree node specifying its children like so:

{{#each children as childData}}
	<:Self {{childData}}/>
{{/each}}

And also, as mrkishi alluded to above, passing arbitrary data to an arbitrary component:

<:Component {name} {{childData}}/>

Neither of these use cases would be inhibited with any of these restrictions in place:

  • not allowed for non-component elements
  • no other attributes can specified on the element
  • only a property name allowed (which can be a computed property), not an arbitrary expression evaluating to an object

Without being familiar with the Svelte codebase, I speculate that these restrictions could make this feature easier to implement. However, to prevent the rules for double curly braces from getting too convoluted, I think a feature with the above restrictions would need non-double curly brace syntax, such as a data directive:

<:Component {name} data:childData/>

@mrkishi
Copy link
Member

mrkishi commented Feb 7, 2018

I've been experimenting on this recently and, as much as I really want it, I don't know if the flexibility of a general solution is really worth the added complexity on the compiler.

By general solution, I mean one with no restrictions to the number of ...{{target}} attributes (and thus, proper attributes deduplication).

(note: assume there are better solutions for everything I mention here! I just couldn't see it)

I mean, it's not that bad and it's certainly not much of a runtime cost (especially since it'd only affect components which actually use it -- thanks disappearing framework!). Still, the attributes precedence algorithm is quite noisy on the output.

Since one of Svelte's main principles is reacting only to data changes (as opposed to re-render the whole app with the full state all the time), we have no option but to cache every single attribute value at different precedence layers in order to correctly deal with property drops. Take a look at this gist for what that might be.

On the other hand, we could impose restrictions to the feature in order to avoid these hard paths. For instance, if we agreed to allow at most one spread attribute per tag and to always favor statically defined attributes, we could have this much simpler variation instead.

While the general version is much more flexible, I think most use-cases would be well served by the restricted variant. It'd still be possible to merge objects with ...{{Object.assign({}, props1, props2)}}, so the only downside I'm seeing is being able to provide defaults as static attributes with spread overrides.

Since spreads will most likely be usually considered an anti-pattern, I'd argue we could start with the restricted version and then evaluate if anything else is needed.

/cc folks interested in the feature and @Rich-Harris,

Do you think that's a worthwhile path? If so, I'll work on a preliminary PR (which I expect will need some love from maintainers, sorry!).

--
I actually just thought of checking out what Vue does, and v-bind="data" has these exact same restrictions!

@TehShrike
Copy link
Member

I haven't absorbed the code snippets, but "allow at most one spread attribute per tag and to always favor statically defined attributes" sounds eminently reasonable to me.

That wouldn't get anywhere near stomping on the use cases I can imagine.

@TehShrike
Copy link
Member

TehShrike commented Mar 27, 2018

Since originally talking about the spread feature, I've started to feel that the most Sveltey implementation of this syntax would be

<Component spread:props />
<Component spread:someObjectInTheScope />

as opposed to

<Component {{...props}} />
<Component {{...someObjectInTheScope}} />

not a big deal, but looking back over this thread I see it hasn't been brought up before, so does anyone else have a thought?

#1248 implements {{...props}}, so I figured I should bring it up quick.

@mrkishi
Copy link
Member

mrkishi commented Mar 27, 2018

I thought about it, but I think for spread it'd be nice to support expressions instead of a name.

For actions, one can do use:actionName="'expression'", but for spread, how would that look? I think we'd be forced to use a dummy name in order to bind to the expression, eg. spread:props="props".

IMO, it's important to have expressions here because of the 'one spread per tag' limitation. Users could need to merge multiple objects before spreading, making spread:all="Object.assign({}, defaults, props)" useful.

If we did lift that limitation, we'd be able to spread:defaults spread:props, but it'd open that can of worms of dynamic properties fighting for priority -- which we'd have to keep track of at runtime.

@Rich-Harris
Copy link
Member

If we end up replacing {{ and }} in templates with { and } (which I'd like to), then {{...obj}} would end up as {...obj}, which React users will find nice and familiar. Agree that we need to support arbitrary expressions.

@Rich-Harris
Copy link
Member

Also, I'm starting to wonder if maybe it's okay to have multiple spreads? If the alternative to

<Foo {...a} {...b} {...c} d={42}>

is that people will write

<Foo {...Object.assign({}, a, b, c)} d={42}>

anyway, then do we gain anything with the constraint?

@PixievoltNo1
Copy link

With the latter of those two forms, it's obvious that updating a, b, or c will cause all 3 to be re-iterated. If we had the former but made it equivalent to the latter, devs doing weird things like having expensive-but-enumerable getters, or deliberately updating objects without feeding them to set(), would run into "Svelte bugs" when updating one of the other spreaded objects.

@mrkishi
Copy link
Member

mrkishi commented Mar 27, 2018

It wouldn't be the end of the world, but at runtime we'd have to track each spread separately just in case something was deleted from b. Not only the spread values -- if we allowed multiple spreads in any order, then we'd also have to track regular properties, since modifying spread objects would potentially use them:

<Foo a={42} {...b} c={1}>
<!--
    What happens if `b` is `{ a: 43, c: 2 }`?
    What about deleting `a` from `b`?
-->

At least that's what I gathered from the exploration, but maybe I'm missing something obvious. 😄

Because it'd be simpler to have a single spread and users would still be able to use those expressions to intentionally spread multiple objects at once, I thought it was a worthwhile limitation.

@Rich-Harris
Copy link
Member

The more I think about this, the more I think that maybe React already has the right solution to this particular issue, and we're tying ourselves in knots trying to avoid unnecessary re-rendering. Basically, this JSX...

<Foo {...a} b={1} {...c} d={2}/>

...translates to this JS:

React.createElement(Foo, _extends({}, a, { b: 1 }, c, { d: 2 }));

If we did the same thing (i.e. bail out of the optimisation allowed by knowing the attribute names ahead of time), our lives would get a lot simpler, and the performance characteristics would be pretty similar in all but somewhat contrived scenarios, I think. (It'll still be faster than React, anyway!)

Might have a quick crack at that and see how straightforward it is to implement on top of #1248.

@mrkishi
Copy link
Member

mrkishi commented Mar 28, 2018

@Rich-Harris But in Svelte, we're talking about translating those attributes into a set. Wouldn't that mean observers for b and d would have to fire when a or c changed? Because I think that would be pretty undesirable.

@PixievoltNo1
Copy link

I have no familiarity with Svelte internals, so much of your talk about what they would do eludes me. I'm just concerned with developer ergonomics - currently, when you do .set({prop: myObject}), myObject gets read once and won't be heard from again until the next .set({prop: myObject}), which can be weird but at least it's consistent.

It sounds like you're headed for a design in which you can do <Foo {{...myObject}} {{...somethingElse}}>, and changing somethingElse causes myObject to be re-read - and this would happen behind the developer's back, so if they're using myObject as anything more than an immutable data store, they could make problems for themselves without knowing it.

@TehShrike
Copy link
Member

set does check every key in the object passed in before actually applying that particular change, so I think that case would be fine.

@mrkishi
Copy link
Member

mrkishi commented Mar 28, 2018

Yeah, but set bails on objects since they could have changed internally.

If we had

<Foo {...a} b={objB} {...c} d={objD}/>

we couldn't simply merge them all into a single foo.set({...a, b: objB, ...c, d: objD}) because it wouldn't know if objB and objD did in fact change. I guess re-rendering wouldn't cause performance issues in most cases, but triggering observers would be a no-go for me.

Unless I misunderstood @Rich-Harris's suggestion, I still think the restriction is valid. Vue, which is closer to Svelte than React, has those same limitations.

@jacwright
Copy link
Contributor

I think each spread would need to be handled separately to avoid observers being triggered unnecessarily, but I don't think that will be a large challenge. foo.set({...a}) should happen only when a changes.

Would we support this use-case? <Foo {{...state}}> and <a {{...state}}> for components to proxy state down to their child components or elements.

@mrkishi
Copy link
Member

mrkishi commented Mar 29, 2018

That's why we were talking about the one spread per tag restriction: when multiple spreads are handled separately, we end up having to track the current state of every property on the tag since we can only know what properties are set by which spread at runtime.

@Rich-Harris
Copy link
Member

I think I've got it. I haven't actually tested this yet, but I feel like there's a good 60% chance it might work: https://gist.github.com/Rich-Harris/4c7784e9c305b9c67019317714ec3fd0. If anyone wants to give it a sanity check before I try and work it into a PR, please do!

Would we support this use-case? <Foo {{...state}}> and <a {{...state}}>

I guess that's the equivalent of {...this.props}. I can see the value of it, though I don't think it should be ...state — the state name shouldn't be 'special' as far as component authors are concerned. Maybe there's something wacky like {...@}? (ugh) Either way, I reckon it's probably best if we come back to that once the rest of this stuff is implemented.

@mrkishi
Copy link
Member

mrkishi commented Mar 30, 2018

Looks sound to me, and it's certainly much cleaner!

@TehShrike
Copy link
Member

aah I'm so excited about this!

If we're picking magical names to represent "the current state", I'd prefer data over state, since that's the method that defines the initial value, so that's what it is in my mind.

I'm not sure if adding a reserved word is better or worse than finding magical characters that happen to be invalid identifiers to represent things. I think I might prefer a new reserved word to a magical character in this case. I dunno

@mrkishi
Copy link
Member

mrkishi commented Mar 30, 2018

I took a closer look and the only thing that stood out is that Object.keys(o) could fail if trying to spread undefined/null. I just don't know if Svelte should handle it, since #each also fails for undefined iterators.

I guess one could argue set({ b_props: undefined }) looks like someone intentionally cleaning it, but so does set({ someArray: undefined }).

@Rich-Harris Rich-Harris mentioned this issue Mar 30, 2018
4 tasks
Rich-Harris added a commit that referenced this issue Apr 1, 2018
@Rich-Harris
Copy link
Member

Now implemented: https://svelte.technology/repl?version=1.60.0&gist=4361c2c3cdeb8b6052240c8be82857a7

If we do end up deciding we need a way to pass down the current props (equivalent to {...this.props} in React), perhaps {{...this}} would be the most appropriate. We can cross that bridge later though.

Re Object.keys(undefined), I think I'm ok with that failing. AFAIK it would also fail in React :)

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 a pull request may close this issue.

8 participants