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

Unable to define some attributes on Custom Elements #3919

Closed
tenphi opened this issue Nov 13, 2019 · 9 comments
Closed

Unable to define some attributes on Custom Elements #3919

tenphi opened this issue Nov 13, 2019 · 9 comments

Comments

@tenphi
Copy link

tenphi commented Nov 13, 2019

Describe the bug
It's unable to define some attributes on 3rd-party Custom Elements.

To Reproduce
Open REPL
I created the simplest Custom Element that do console.log of attribute prefix when connected. But Svelte can't create such attribute because there is a read-only property prefix on HTMLElement prototype. Actually Svelte won't define any attribute if there is a property with the exact name. Instead it will try to assign the value directly to the property.

Expected behavior
It should be possible to create ANY attribute with ANY value that does not contradict the standard.

Information about your Svelte project:

  • Your browser and the version: Chrome 78.0.3904.97

  • Your operating system: MacOS 10.15.1

  • Svelte version: 3.14.0

  • I use Rollup

Severity
I create a UI Framework that based entirely on Custom Elements. That bug is blocking me to use Svelte with it. I love Svelte but I really upset that it contradicts the standard.

@tenphi
Copy link
Author

tenphi commented Nov 13, 2019

Svelte source code related to the issue: https://github.com/sveltejs/svelte/blob/master/src/runtime/internal/dom.ts

export function set_custom_element_data(node, prop, value) {
	if (prop in node) {
		node[prop] = value;
	} else {
		attr(node, prop, value);
	}
}

@stalkerg
Copy link
Contributor

Explanation here #875

@tenphi
Copy link
Author

tenphi commented Nov 13, 2019

I understand why it was implemented this way. But in my opinion it's wrong. As I know, no other frameworks use similar logic for good reasons:

  1. It's not future proof. Introducing any new property on HTMLElement can result in unexpected runtime errors.
  2. This approach uses same property namespace as HTMLElement.prototype which can result in unexpected behavior without any warnings.
  3. It's confusing. Attributes are not properties! They work differently and should have different explicit API to set them.
  4. Framework shouldn't decide which attribute names I can use and which I can't. There is a standard for that.
  5. Migration to Svelte can be tough if many Custom Elements are used.
  6. There are more than hundred (as far as I know) properties in HTMLElement that can have conflict with attribute name.
  7. Some Custom Elements can depend on styling like [attr] { padding: 10px; } but without providing two-way binding with inner prop attr. Standard input element with value attribute doesn't have two-way binding. It's handy and typical but Custom Element with such behavior will be broken while using with Svelte.

It's basically makes usage of Svelte dangerous and not future-proof.

@Rich-Harris
Copy link
Member

tl;dr custom elements are a badly designed primitive and I can't recommend them in good conscience.

There are a few different strategies that frameworks can use to handle this problem:

  • Always set attributes. This is what React does IIRC, and it results in the opposite issue — custom elements that define an interface using props instead of attributes are essentially broken in React. It also means you're limited to passing data that can be stringified.
  • Always use props.
  • Use some heuristic for determining when to use props and when to use attributes. This is what Svelte does, and I think what some other frameworks (Preact, maybe?) also do, because empirically it appears to be the most broadly compatible approach.
  • Invent some new syntax for differentiating between the two. This is what lit-html does, for example (foo="attribute", .bar="prop"). But then you have to either a) introduce an inconsistency between custom elements and normal ones, or b) make people write <input .value={foo}>, which is a bit gross. It forces people to understand distinctions that shouldn't matter in day-to-day situations, and means that the Svelte language is no longer a simple superset of HTML. People proficient in HTML would then have to learn what is essentially something framework-specific.

None of these options are ideal, but it's what's been forced on us by the design of custom elements. (Imagine if we'd designed them in such a way that attributes were simply the inputs to a function that generated initial props when a custom element was instantiated, and from that point forward you only had to worry about props? We could have avoided all this silly confusion.)

You're right that it's not future-proof. Unfortunately, custom elements are themselves not future-proof — any new attribute or property that applies to HTMLElement will potentially conflict with custom elements being used in the wild. Smooshgate, except that we encouraged people to pollute the language!

Unfortunately I just don't think there's a good solution to this. Which, along with other serious shortcomings (global namespace, no SVG support, no SSR — and no, declarative shadow DOM won't fix that), is why I personally advise against their use.

A possible workaround in this case would be to implement prefix on any elements that use it as an attribute:

class Foo extends HTMLElement {
  get prefix() {
    return null;
  }

  set prefix(value) {
    this.setAttribute('prefix', value);
  }
}

@tenphi
Copy link
Author

tenphi commented Nov 18, 2019

Thanks for your answer. I really appreciate all the work you've done on Svelte & Rollup projects. But I don't agree with the thesis that "custom elements are a badly designed primitive". It just wasn't meant for the task the most developers try to solve with it. But it doesn't mean it's bad, useless or something. I created entire Design System and Framework based on native API and in our company we are very VERY happy with it. But in the same time we understand that our approach is barely popular. If you're interested and have like 10 minutes I can show you some interesting applications of Custom Elements on call.

As for this issue, I understand that there is a very little chance something can be changed here. But here is my recommendation:

  1. We can check if property is inherited from HTMLElement/Element then apply value to the attribute in this case. Looks like safe option.
  2. We can also check static observedAttributes for CE attributes, but it can break some current behaviors I think.

At least we should discuss this options.

For those who are faced such problem right now I would recommend to create standalone utility to fix 3rd party Custom Elements.

The following instructions are only for Svelte users:

Get a list of all props that should be redeclared if you want to use it with CE:

ALL_ELEMENT_PROPS = Object.keys(HTMLElement.prototype).concat(Object.keys(Element.prototype))

Get a list of all global attributes that are supposed to be untouched by CE logic:

const GLOBAL_ATTRS = ['accesskey', 'autocapitalize', 'class', 'contenteditable', 'contextmenu', 'dir', 'draggable', 'dropzone', 'hidden', 'id', 'itemprop', 'lang', 'slot', 'spellcheck', 'style', 'tabindex', 'title', 'translate']

Get a list of CE attributes:

const ceAttrsList = CustomElement.observedAttributes;

So we can create the utility that will add getter/setter for every CE attribute only if it's needed:

  1. Attribute is used for EC logic, declared in observedAttributes.
  2. Property with such name is declared in ALL_ELEMENT_PROPS.
  3. Property with such name is NOT declared in Element's prototype.
  4. Attribute is not in GLOBAL_ATTRS list. (show warning if it is)
  5. Apply custom restrictions based on internal logic of your project.

Don't forget to make a camelCase transformation for attribute name: some-prop Attribute -> someProp Property.

It's won't make any Custom Element 100% compatible with Svelte, but it will cover the most of the cases.

Good luck!

@alkismavridis
Copy link

alkismavridis commented Dec 28, 2019

@Rich-Harris I think the approach "Use some heuristic for determining when to use props and when to use attributes" is not a good choice. It basically gives users no control whatsoever over the binding process, and this control can sometimes be crucial.

Correct me if I am wrong, but I have the feeling that your heuristic solution depends on the authors for providing a specific implementation policy for their w.c., but leave users completely powerless in cases where the authors, for whatever reason, chose to rely on an attribute instead of an existing field.
In such cases, the W.C. would be unusable under svelte.
Or, how could a user deal with such a case?
What would be his way out?
Author of the w.c. decided that is should work like "A", svelte decided that it should work like "Z", and I, the user, am stuck in the middle wondering what to do.
Is there something that I miss?

My suggestion:
Go with the fourth option that you mentioned, something that gives us the option to control the process, in cases that we absolutely need to. Give us this option, even if we use it rarely.

Let the gross syntax be the exception, where users would need explicit control.
Let the elegant syntax be the default, heuristic behaviour.
Something like that:

.foo="force prop"  //for users who, for whatever reason NEED to bind to prop. Replace the . symbol with whatever you think makes more sense.
!foo="force attribute"  //for users who, for whatever reason NEED to bind to attribute. Replace the ! symbol with whatever you think makes more sense, like attr:foo etc.
foo="I do not care about the distinction, I let svelte decide". //Elegant syntax that implements the heuristic behaviour, or any other default behaviour you think makes sense.

This way, we let things as they are for 99% of the use cases, but we leave the door open for any use case that will require more explicit control.

Thanks in advance.

@alkismavridis
Copy link

alkismavridis commented Dec 29, 2019

Ok, I think I found a workaround to achieve explicit control on the attribute/prop issue.
Here is it, in case somebody needs to gain such control:

  1. I create a small file with util functions to be reused (here I use ts, but this can be very easily converted to js). I export two functions: attr and prop.
function updateAttribute(node:HTMLElement, name:string, value:any) {
  if (value==null || value==+false) {
    node.removeAttribute(name);
  }
  else node.setAttribute(name, value+"");
}

//calls setAttribute or removeAttribute
export function attr(node: HTMLElement, entry: [string, any]) {
  if(entry) {
     updateAttribute(node, entry[0], entry[1]);
   }

  return {
    update(updated: [string, any]) {
      if(!updated) return;
      updateAttribute(node, updated[0], updated[1]);
    }
  }
}

//assigns the given value to the given key
export function prop(node: HTMLElement, entry: [string, any]) {
  if(entry) {
     node[ entry[0] ] = entry[1];
  }

  return {
    update(updated: [string, any]) {
      if(!updated) return;
      node[ updated[0] ] = updated[1];
    }
  }
}
  1. I control the binding process with a use: directive:
//in my script tag:
import {attr, prop} from "../utils/WebComponentUtils";

//in my markup:
<my-web-component use:prop={["foo", bar]} /> //performs el.foo = bar;
//or
<my-web-component use:attr={["foo", bar]} />   //performs setAttribute("foo", bar+"") or removeAttribute("foo")

It works fine (including updating when the values change).
Still, the syntax is to verbose, and creating an array looks a bit ugly.
Would be super cool if we could write something like:

<my-web-component .foo={bar} />  //or prop:foo or whatever
<my-web-component  !foo={bar} /> //or attr:foo or whatever

and achieve the same result without the need of creating arrays and implementing custom util functions.

@stale
Copy link

stale bot commented Jun 27, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Jun 27, 2021
@stale
Copy link

stale bot commented Jul 11, 2021

This issue has been closed as it was previously marked as stale and saw no subsequent activity.

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

No branches or pull requests

5 participants