Add spread for attributes #280

PaulBGD · 2017-02-06T03:41:16Z

This will close #195.

It implements spread as:

<input {{options}}/>

into

<input type="text" value="Hello World"/>

where options is:

{
  "type": "text",
  "value": "Hello World"
}

This is a work in progress until I've finished the implementation for Components.

Some things that might need to be discussed:

When the attributes are static, you can throw an error when they try to use 2 of the same attribute. When the attributes are dynamic through spreading, there's no such guarantee.
In Spread properties and special *state* property #195 it was suggested to use _ to spread all data, which would look like {{_}}. Is this the syntax we want to use?

PaulBGD · 2017-02-06T19:47:49Z

So I just pushed a commit that uses a component with attribute spread and it worked fine.. I might be confused on what constitutes a component because I thought I'd have to implement the Spread attribute inside of addComponentAttributes. So if it works with components then I guess this PR is ready.

Swatinem · 2017-02-08T12:31:42Z

src/generators/server-side-rendering/visitors/Element.js

@@ -11,6 +11,10 @@ export default {
 		let openingTag = `<${node.name}`;

 		node.attributes.forEach( attribute => {
+			if ( attribute.type === 'Spread' ) {
+				openingTag += ` \${Object.keys( root.${attribute.name} ).map( prop => \`\${prop}="\${root.${attribute.name}[prop]}"\` ).join( ' ' )}`;


I think this needs some kind of escaping. But then again, the normal attribute code below has no escaping either.

So this code was written a little later than the generator code and while it works, I think it's really sloppy.

Swatinem · 2017-02-08T12:36:35Z

src/shared/dom.js

@@ -22,6 +22,20 @@ export function teardownEach ( iterations, detach, start ) {
 	}
 }

+export function spreadAttributes ( node, obj ) {
+	if (obj !== null && typeof obj === 'object') {
+		for ( var property in obj ) {


What is the policy on browser compatibility?
I personally prefer iterating over Object.keys instead of for-in + hasOwnProperty.

Also, I would personally prefer an early return instead of indenting, but that is a matter of taste :-)

Wow I for some reason thought Object.keys was ES6, but it's actually ES5! Definitely will use that.

Oh and on browser compatibility, the only browser with significant market share that doesn't support ES5 is IE8 at 3% marketshare. But I honestly think that 8 years later we can ship ES5 features to browsers. + there's plenty of polyfills for ES5 at this point.

I think it's reasonable to expect Object.keys to exist, especially since it's easily polyfilled if you absolutely have to support IE8 or whatever — though a for-in is quicker, no?

(Related: the compiler generates code with Object.assign which is less widely-supported than Object.keys — again, can be easily polyfilled but I wondered if there should be an internal assign helper instead.)

According to this Object.keys + for loop is faster: https://jsperf.com/object-keys-vs-for-in-with-closure/3

Rich-Harris · 2017-02-08T20:50:34Z

Added support for updates and components in PaulBGD#1, which is based on this branch.

Having said that, how strongly do you both feel about this addition? I can't help but wonder if it's a bit of a footgun, since it means less explicit templates and reduced static analysis ability, which feels like it might be a slippery slope (e.g. could potentially impact future optimisations?). Isn't the analogous {...props} in React considered an anti-pattern?

Swatinem · 2017-02-08T21:00:51Z

Isn't the analogous {...props} in React considered an anti-pattern?

Not that I know of. For some you want to use object spread/rest to filter out the props you do not want to pass to the child. Otherwise spread in react is perfectly fine.

PaulBGD · 2017-02-08T21:42:15Z

So one of the issues that I noticed was that being dynamic means that you don't know what attributes are being passed. Since the common use-case for this is for passing multiple arguments from data, I wonder if we could do something where we specify the properties of data that should be passed to the child.

One thing we could do is a syntax like

<input pass:type pass:value />

Where it passes values from the parent, but that's a little verbose still.

I suppose the best way to statically analyze the object to make this PR work is to specify the parameters.
For example we could add something to the component object something like

<elem {{ab}}/>

export default {
  data() {
    return { a: 1, b: 2, c: 3 }
  },
  spread: {
    ab: ['a', 'b']
  }
}

and that's the easiest way I can think of making it statically analyzable as-is.

Updating and component-friendly spread attributes

PaulBGD · 2017-02-15T21:39:18Z

Any additional thoughts on this? One thing suggested if we're sure we want to go the dynamic route is allow passing all data.

Rich-Harris · 2017-02-15T22:08:24Z

To be totally honest I'm still not totally convinced of the utility of this change — if we're talking about things like adding a spread property to work around the potential pitfalls then are we sure that it's worth all the extra maintenance, generated code, documentation requirements etc? It doesn't seem to me like we really gain that much from it.

PaulBGD · 2017-02-16T03:16:37Z

When I use svelte and I want to have custom styles for buttons or special effects for them, I create a class called ButtonComponent or ButtonWrapper. How can I pass type, disabled, name, value, and all other fields to the button without checking if each attribute is being passed?

Rich-Harris · 2017-02-16T15:41:17Z

Isn't that just this?

<!-- Main.html -->
<ButtonComponent type='reset' name='a button!'>some label</ButtonComponent>

<!-- ButtonComponent.html -->
<button type='{{type}}' disabled='{{disabled}}' name='{{name}}' value='{{value}}'>
  {{yield}}
</button>

<script>
  export default {
    data: () => ({
      type: 'button',
      disabled: false,
      name: '',
      value: ''
    })
  };
</script>

In other words, as a component author I think it makes sense that you have to anticipate the different attributes that could get passed, otherwise you could wind up with some unpredictable behaviour.

If it's the foo='{{foo}}' repetition that's a problem, then maybe we could address that directly without adding complexity, e.g. this...

<button :type :disabled :name :value>{{yield}}</button>

...where :foo is equivalent to foo='{{foo}}' (like bind:foo without the bind). Still extra stuff to learn, and I can imagine someone turning their nose up at it when they first read a Svelte template, but it's nice and unambiguous.

Have also wondered about replacing {{ with just {, though that'd obviously be a breaking change. Would make templates look a bit more JSX-y:

<button type={type} disabled={disabled} name={name} value={value}>{yield}</button>

PaulBGD · 2017-02-16T16:09:09Z

I think I agree and I like the :attribute syntax.

Swatinem · 2017-02-16T16:15:59Z

My original proposal in #195 was partly tailored towards iteration with components.

Since {{each}} only supports binding to identifiers and no destructuring, the proposed shorthand syntax <foo :bar /> would not really work.

Rich-Harris · 2017-02-16T17:04:52Z

Since {{each}} only supports binding to identifiers and no destructuring

What if it did support destructuring?

{{#each items as { description, completed } }}
  <Item :description :completed/>
{{/each}}

Granted, there's a lot of curly braces there, and I haven't thought through the ramifications of this at all, but does that help solve the problem?

Swatinem · 2017-02-16T17:23:28Z

It does yes. You still have to enumerate all the props though, but that might as well force the author to think more about it and come up with a solution that does not rely on tons of props.

PaulBGD · 2017-03-01T01:45:32Z

I'm going to close this since it conflicts with the main branch and because doing dynamic spreading of attributes would make things like #310 not possible.

TehShrike · 2017-06-07T03:32:22Z

I feel bad necroing this PR, but I'd like to raise a voice in favor of spread operators. Without them, I have to add hacky className and style parameters explicitly (see https://github.com/TehShrike/svelte-querystring-router/blob/af5fa0c3dd33d45655a89206af4ebbcf94463439/Link.html#L4-L5 ) and just hope that the consumer never has a real need for any data attributes or anything.

I didn't think spread properties were an anti-pattern in React - maybe some people avoided them, but I found them very useful in making general components.

PaulBGD · 2017-06-07T18:12:47Z

The big issue I've seen with having a spread all (which I think is what most people want, with the ability to override certain attributes) is that Svelte doesn't know at compile time if the component will be used by the DOM and/or by another component. This means we have to pass all the attributes with an object at runtime, which incurs a slight cost.

I think that the feature is useful though, especially if we do want to support aria attributes. Right now it's very annoying to proxy all the events a node can have, as well as passing data-** attributes. This makes creating libraries near impossible, because you literally cannot know all the possible attributes that will be passed.

bestguy · 2017-07-24T00:17:52Z

I'm running into this exact issue with a lib of mine (https://github.com/bestguy/sveltestrap) where I wrap of Bootstrap components as Svelte components, and it's really, really difficult to manually specify the on: events, aria-, etc attributes

PaulBGD added 3 commits February 5, 2017 21:36

Add spread for attributes (elements only atm)

390bc78

This is why you lint

d4c671d

Add test using components

c63a253

PaulBGD changed the title ~~[WIP] Add spread for attributes~~ Add spread for attributes Feb 7, 2017

Swatinem reviewed Feb 8, 2017

View reviewed changes

update spread attributes

535b896

Rich-Harris mentioned this pull request Feb 8, 2017

Updating and component-friendly spread attributes PaulBGD/svelte#1

Merged

Merge branch 'master' into spread-attribute-updates

59234c0

handle spread attributes in SSR

d83182c

Merge pull request #1 from sveltejs/spread-attribute-updates

24212cf

Updating and component-friendly spread attributes

PaulBGD mentioned this pull request Feb 15, 2017

Appending classes to nested components #289

Closed

PaulBGD closed this Mar 1, 2017

Rich-Harris mentioned this pull request Mar 16, 2017

[WIP] Unified attributes and bindings #380

Merged

mrkishi mentioned this pull request Jan 24, 2018

Spread properties and special *state* property #195

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add spread for attributes #280

Add spread for attributes #280

PaulBGD commented Feb 6, 2017

PaulBGD commented Feb 6, 2017

Swatinem Feb 8, 2017

PaulBGD Feb 8, 2017 •

edited

Loading

Swatinem Feb 8, 2017

PaulBGD Feb 8, 2017

PaulBGD Feb 8, 2017

Rich-Harris Feb 8, 2017

PaulBGD Feb 11, 2017

Rich-Harris commented Feb 8, 2017

Swatinem commented Feb 8, 2017

PaulBGD commented Feb 8, 2017 •

edited

Loading

PaulBGD commented Feb 15, 2017

Rich-Harris commented Feb 15, 2017

PaulBGD commented Feb 16, 2017 •

edited

Loading

Rich-Harris commented Feb 16, 2017

PaulBGD commented Feb 16, 2017

Swatinem commented Feb 16, 2017

Rich-Harris commented Feb 16, 2017

Swatinem commented Feb 16, 2017

PaulBGD commented Mar 1, 2017

TehShrike commented Jun 7, 2017

PaulBGD commented Jun 7, 2017

bestguy commented Jul 24, 2017

Add spread for attributes #280

Add spread for attributes #280

Conversation

PaulBGD commented Feb 6, 2017

PaulBGD commented Feb 6, 2017

Swatinem Feb 8, 2017

Choose a reason for hiding this comment

PaulBGD Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Swatinem Feb 8, 2017

Choose a reason for hiding this comment

PaulBGD Feb 8, 2017

Choose a reason for hiding this comment

PaulBGD Feb 8, 2017

Choose a reason for hiding this comment

Rich-Harris Feb 8, 2017

Choose a reason for hiding this comment

PaulBGD Feb 11, 2017

Choose a reason for hiding this comment

Rich-Harris commented Feb 8, 2017

Swatinem commented Feb 8, 2017

PaulBGD commented Feb 8, 2017 • edited Loading

PaulBGD commented Feb 15, 2017

Rich-Harris commented Feb 15, 2017

PaulBGD commented Feb 16, 2017 • edited Loading

Rich-Harris commented Feb 16, 2017

PaulBGD commented Feb 16, 2017

Swatinem commented Feb 16, 2017

Rich-Harris commented Feb 16, 2017

Swatinem commented Feb 16, 2017

PaulBGD commented Mar 1, 2017

TehShrike commented Jun 7, 2017

PaulBGD commented Jun 7, 2017

bestguy commented Jul 24, 2017

PaulBGD Feb 8, 2017 •

edited

Loading

PaulBGD commented Feb 8, 2017 •

edited

Loading

PaulBGD commented Feb 16, 2017 •

edited

Loading