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

Passing data to custom elements #875

Closed
Rich-Harris opened this Issue Sep 27, 2017 · 14 comments

Comments

Projects
None yet
7 participants
@Rich-Harris
Member

Rich-Harris commented Sep 27, 2017

When passing data to a custom element, there are two possibilities — properties, or attributes.

Right now, Svelte doesn't have any understanding of custom elements, so on encountering something like this...

<custom-element foo='bar'></custom-element>

...it will look here, fail to find a property corresponding to the foo attribute, and fall back to writing this:

setAttribute(custom_element, "foo", "bar");

That's sub-optimal, because it means we can't pass down non-string values — especially not objects and arrays. For those, we need to use props.

At the same time, always using props could be a problem for custom elements that expect attributes to change. It's also a problem if a custom element has CSS like this, say...

:host([active]) {
  font-weight: bold;
}

...because then if you have a situation like this...

<custom-element active='{{thing === selected}}'></custom-element>

...the prop will be maintained correctly but the attribute won't ever change, meaning the styles will never apply.

A possible solution

Do both. Or rather, always set a prop, but also set an attribute if the value is primitive.

So the example at the top would become this:

custom_element.foo = "bar";
setAttribute(custom_element, "foo", "bar");

For dynamic values, it would be more like this:

custom_element.foo = state.bar;

if (typeof state.bar === "boolean" || typeof state.bar === "undefined") {
  if (state.bar) {
    setAttribute(custom_element, "foo", "");
  } else {
    removeAttribute(custom_element, "foo");
  }
} elseif (typeof state.bar === "string" || typeof state.bar === "number") {
  setAttribute(custom_element, "foo", state.bar);
}

Needless to say, all that logic would live in a helper.

kebab-case vs camelCase

Since props can't be kebab-case, I propose that we translate kebab to camel when setting props:

<custom-element the-answer='{{fortyTwo}}'></custom-element>
custom_element.theAnswer = state.fortyTwo;
setCustomElementAttribute(custom_element, "the-answer", state.fortyTwo);

This might seem slightly messy, but I think it's the most pragmatic way to deal with this stuff, and the way that will result in the least surprising behaviour for the largest number of people. No-one ever said custom elements were particularly well designed. (Actually, lots of people did, but most of them work for Google.)

Thoughts?

@Rich-Harris

This comment has been minimized.

Member

Rich-Harris commented Sep 27, 2017

@evs-chris pointed out that we can see what props the custom element is expecting at runtime. We could potentially do that, and fall back to setting attributes if the prop doesn't exist.

@zigomir

This comment has been minimized.

zigomir commented Sep 30, 2017

I'm trying to pass attribute in my index.html file to custom element like so

<custom-element name="test"></custom-element>

and I'm getting

bundle.js:297 Uncaught TypeError: this.set is not a function
    at HTMLElement.attributeChangedCallback (bundle.js:297)
    at bundle.js:301

is this not supported yet or I don't have a correct setup? I'm using rollup-plugin-svelte and option customElement: true

Svelte component looks like this

<h1>Hello {{ name }}!</h1>

<script>
  export default {
    tag: 'custom-element'
  }
</script>

sorry for hijacking this issue.

@marianoviola

This comment has been minimized.

marianoviola commented Oct 1, 2017

@Rich-Harris even if messy, your solution seems a good compromise to get immediate interoperability with custom elements attributes and properties.

In this regard, it would be nice if soon or later we could add Svelte to Custom Elements Everywhere with the ultimate goal to pass all the tests 🤓

@eddyloewen

This comment has been minimized.

eddyloewen commented Dec 18, 2017

@zigomir Have you found a solution to your problem? I'm getting the same error message

@zigomir

This comment has been minimized.

zigomir commented Dec 18, 2017

@CH-RhyMoore

This comment has been minimized.

CH-RhyMoore commented Feb 13, 2018

@zigomir @eddyloewen I'm having the same problem.

When I use svelte to compile custom elements, the built file is in this order:

function create_main_fragment () {...}

class MyElement extends HTMLElement {
  ...
  // set method isn't defined here, but mixed in below
  attributeChangedCallback(attr, oldValue, newValue) {
		this.set({ [attr]: newValue });
	}
}

customElements.define("my-element", MyElement);

assign(MyElement.prototype, {
  ...
 	set: set,
 	_set: _set
  ...
 }, {...}
});

...
function set () {...}
...
function _set() {...}
...

export default MyElement;

If I move the call to define the custom element down below where the set method is mixed in with the prototype, I no longer get the error.

function create_main_fragment () {...}

class MyElement extends HTMLElement {
  ...
 // set method isn't defined here, but mixed in below
  attributeChangedCallback(attr, oldValue, newValue) {
		this.set({ [attr]: newValue });
	}
}

// previously here

assign(MyElement.prototype, {
  ...
 	set: set,
 	_set: _set
  ...
 }, {...}
});

...
function set () {...}
...
function _set() {...}
...

// now here
customElements.define("my-element", MyElement);

export default MyElement;

I'm a bit surprised by this, even though I can outline a series of events that might lead to it (sections 2.4-2.6 of https://www.w3.org/TR/custom-elements/#custom-elements-api). Still, I wouldn't expect the attributeChangedCallback to be able to reliably sneak in any runs before the assign call finishes filling in the required methods, based on my understanding that the effects of calls to define and then upgrade the element uses are async. But, for me, the error is very reliable.

The way I'd expect this to process, by metaphor, is like this:

setTimeout(function(){console.log("a");},0);
console.log("b");

This would go "b a", not "a b". So I have to be wrong about something, I'm just not sure which part.

Still, as a temporary palliative, you might be unblocked by hacking this reorder in.

@CH-RhyMoore

This comment has been minimized.

CH-RhyMoore commented Feb 14, 2018

Also, @Rich-Harris I like your suggestions. FYI AFAIK Polymer and Skate both allow reflection to the attribute via a prop config, but don't do so by default.

skatejs/skatejs#838 https://developers.google.com/web/fundamentals/web-components/customelements#reflectattr

@alindsay55661

This comment has been minimized.

alindsay55661 commented Jul 13, 2018

The decision to mirror between properties and attributes really lies with the custom element author, not the framework. Svelte shouldn't be concerned with whether the binding is to attributes or properties—this digs too deep into the implementation and cares too much about the internal workings of a given custom element. The css example you cited is not something Svelte needs to know or care about, but rather something the custom element author, and potentially consumer, should know and care about.

always using props could be a problem for custom elements that expect attributes to change.

The custom element can be designed to handle this robustly by using mirroring rather than expecting frameworks everywhere to behave uniformly. Alternatively, is it really so wrong to have a custom element that uses attributes and props independently (i.e. attributes for the purpose of css only)? The question then shifts from:

What should Svelte do exclusively and automatically?

to

Should Svelte provide independent binding options for properties and attributes?

If the answer to the later question is yes, then you return binding control to the consumer—the only decision maker with enough information to truly know whether you should bind to an attribute or a property. Consumers are then free to bind as each custom element requires... and for robust elements it won't matter. But either way, this isn't really Svelte's concern.

Rich-Harris added a commit that referenced this issue Aug 5, 2018

@Rich-Harris

This comment has been minimized.

Member

Rich-Harris commented Aug 5, 2018

Thanks for the feedback everyone. I finally got round to working on this — see #1636.

@alindsay55661 On reflection I think you're right that it should be down to custom elements to reflect props back to attributes. I think the best approach is to use props if node[prop] exists and fall back to setting attributes, per Chris's suggestion (#875 (comment)) — this makes it easier to use aria-*, class, style, data-* attributes etc.

@zigomir @eddyloewen @CH-RhyMoore apologies for missing those bug reports earlier. What's the simplest complete reproduction? There's a test that seems like it should cover that scenario, but perhaps I'm missing something? Could you open a separate issue if so please? Thanks

@zigomir

This comment has been minimized.

zigomir commented Aug 5, 2018

@Rich-Harris I'm trying out this right now (with Svelte 2.9.9) and I can't reproduce it anymore. I'm able to pass attributes with custom element just fine. Although computed property function will be triggered each time for every attribute. For example if when I use two attributes, computed property will be called like this:

computed: {
  method({ attr1, attr2}) {
    console.log(attr1, attr2)
  }
}
undefined undefined
"foo" undefined
"foo" "bar"

when you use custom element in your html as:

<custom-element attr1="foo" attr2="bar"/>

is this intended?

Another unfortunate side effect that I'm seeing is that when I use customElement: true in Svelte's compile options css file will be empty.

@Rich-Harris

This comment has been minimized.

Member

Rich-Harris commented Aug 5, 2018

Ah good, it must be fixed then.

is this intended?

I would use the word 'expected' rather than 'intended'... this is an unfortunate thing about custom elements. It would be easy enough to generate code that batched up property changes...

class SvelteComponent extends HTMLElement {
  get foo() {
    return this.get().foo;
  },
  set foo(foo) {
    if (!this._pendingChanges) {
      const pending = this._pendingChanges = {};
      Promise.resolve().then(() => {
        this._pendingChanges = null;
        this.set(pending);
      });
    }
    this._pendingChanges.foo = foo;
  },
  // ...repeat for each property
}

...but it could easily lead to incorrect behaviour if setting properties is supposed to have some immediate effect:

element.foo = 'potato';
console.log(element.offsetWidth); // incorrect, because state change is pending

You'd have to get super-sophisticated to make sure that reading properties like offsetWidth caused any pending changes to flush.

Another unfortunate side effect that I'm seeing is that when I use customElement: true in Svelte's compile options css file will be empty.

This is because it uses Shadow DOM — if you look at the generated CSS you'll see that it's creating a <style> tag inside the element. There's an issue for generating custom elements without shadow DOM — #1168

@zigomir

This comment has been minimized.

zigomir commented Aug 6, 2018

👍 I see, thanks for the response!

Another question; is there a way to tell through svelte that custom property attribute is a number and not a string?

<custom-element start="1" />

Svelte will generate a getter as

get start() {
  return this.get().start;
}

but this will always return a string.

I know I can do

const el = document.querySelector('custom-element')
el.start = 1

but that just doesn't look so good :)

@Rich-Harris

This comment has been minimized.

Member

Rich-Harris commented Aug 6, 2018

In raw HTML? I don't think so, no. In a Svelte component you could do this, and it should work:

<custom-element start={1} />

Rich-Harris added a commit that referenced this issue Aug 8, 2018

Merge pull request #1636 from sveltejs/gh-875
use props when passing data to custom elements (#875)
@Rich-Harris

This comment has been minimized.

Member

Rich-Harris commented Aug 8, 2018

Released 2.9.11 with better custom element handling. We now pass all the tests on custom-elements-everywhere; I've raised a PR with them webcomponents/custom-elements-everywhere#257

@Rich-Harris Rich-Harris closed this Aug 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment