-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Comments
The |
Hm, I thought of |
Will close this now we have shorthand attributes ( |
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. |
@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) |
No worries, I was just thinking that this issue should probably get necro'd back to open. |
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 (this explanation is not gonna be succinct, but I couldn't express this clearly, sorry!) Let's start with a simple component Let's move to That looks perfect, until you realize you've lost the ability to use the routes' When you have 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 This is what our example could look like with spread: repl. Note how we regain the routes' data flexibility while keeping If you're still open for PRs, I'll take a look into it -- but I may need some help with Svelte's internals. 😀 |
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:
And also, as mrkishi alluded to above, passing arbitrary data to an arbitrary component:
Neither of these use cases would be inhibited with any of these restrictions in place:
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
|
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 (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 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 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. |
Since originally talking about the spread feature, I've started to feel that the most Sveltey implementation of this syntax would be
as opposed to
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 |
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 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 If we did lift that limitation, we'd be able to |
If we end up replacing |
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? |
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. |
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 <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. |
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. |
@Rich-Harris But in Svelte, we're talking about translating those attributes into a |
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 It sounds like you're headed for a design in which you can do |
|
Yeah, but If we had <Foo {...a} b={objB} {...c} d={objD}/> we couldn't simply merge them all into a single 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. |
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. Would we support this use-case? |
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. |
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!
I guess that's the equivalent of |
Looks sound to me, and it's certainly much cleaner! |
aah I'm so excited about this! If we're picking magical names to represent "the current state", I'd prefer 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 |
I took a closer look and the only thing that stood out is that I guess one could argue |
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 Re |
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:
And having a special identifier for the complete state object would make decorator components really awesome, like so:
The text was updated successfully, but these errors were encountered: