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

[idea] Make use of observedAttributes optional #565

Closed
trusktr opened this issue Sep 13, 2016 · 45 comments
Closed

[idea] Make use of observedAttributes optional #565

trusktr opened this issue Sep 13, 2016 · 45 comments

Comments

@trusktr
Copy link

trusktr commented Sep 13, 2016

I think it might be nice that if the observedAttributes static property is not present on the constructor, then for attributeChangedCallback to behave as in v0, where it is called for all attribute changes.

Unless I misread the spec, there is only a single algorithm that uses observedAttributes in the spec, and that algorithm doesn't specify what happens when it is undefined, it only specifies what happens when it is "not undefined".

Furthermore, Section 2.6 says

When any of [a custom element's] attributes are changed, appended, removed, or replaced, [the custom element's] attributeChangedCallback is run.

noting the keyword "any" in that sentence.

Based on the above two facts, then there is nothing that states what the behavior should be if attributeChangedCallback is defined while observedAttributes is not. In my opinion, it'd be nice for behavior to remain the same as in v0 where attributeChangedCallback is called for every attribute that is changed, so that migration from v0 to v1 is easy. Plus, it's easy to opt in to the new behavior anyways by simply defining the observedAttributes property.

Related (reason why I opened this issue): WebReflection/document-register-element#76

@trusktr trusktr changed the title [idea] Static observedAttributes property being optional. [idea] Make use of observedAttributes optional Sep 13, 2016
@matthewp
Copy link

Isn't the reason for observedAttributes performance?

@JanMiksovsky
Copy link

While many things changed from Custom Elements v0 to v1, in upgrading elements to v1, I've been surprised that that dealing with observedAttributes has been, by far, the trickiest issue I've grappled with. It can be tedious and error-prone to keep observedAttributes up to date. I've hit numerous bugs that I've eventually traced to the failure to remember to add a new property to observedAttributes. It also complicates the creation of component mixins or subclasses, which need to dynamically insert the attribute they're defining into a list of attributes observed by the base class.

As @matthewp indicates, the motivation behind observedAttributes was performance. Specifically, it was observed that many apps set the style attribute programmatically with multiple invocations:

this.style.width = newWidth;
this.style.height = newHeight;
this.style.display = newDisplay;
...

Each of those style updates would invoke attributeChangedCallback. To avoid this problem, observedAttributes was introduced.

@trusktr's proposal above would help, but I think it'd fall down in the case, e.g., where a mixin/subclass wants to observe an attribute. If a mixin/subclass defined observedAttributes with an attribute it was interested in, this would suddenly break base class behavior that had depended on no observedAttributes being defined in order to get the default behavior proposed above.

Counter-proposal: all attributes except standard attributes always trigger attributeChangedCallback.

  • For custom attributes, it feels like the default behavior should be to invoke attributeChangedCallback.
  • It's rare for a component to want to react to a change in a standard attribute (like style), so forcing declaration of that seems acceptable. The use of observedAttributes could be reserved for observing standard attributes.
  • The name of the property could change to observedStandardAttributes to reflect the refined semantics. If that member included custom attribute names, those would be ignored.

@domenic If I recall, you were the driving force behind the introduction of observedAttributes. Given the challenges we're having using this member, do you think it'd be possible to refine how it works?

@domenic
Copy link
Collaborator

domenic commented Sep 14, 2016

No, I don't think it's possible to change this at this stage.

@JanMiksovsky
Copy link

Drat. Okay, thanks.

@trusktr
Copy link
Author

trusktr commented Sep 14, 2016

On the upside, observedAttributes makes it possible to define a feature in a base class which reflects the defined attributes as properties on the element instance in an official way (as in there is now an official way to specify which attributes an element will have).

Will be playing around with it and seeing how it goes...

@domenic
Copy link
Collaborator

domenic commented Sep 14, 2016

Given how observedAttributes was designed, and how the consensus was explicitly to make it mandatory, I'll close this issue. Happy to continue discussing in the closed thread if people are unable to find the meeting notes and GitHub issues documenting that decision on their own.

@domenic domenic closed this as completed Sep 14, 2016
@rniwa
Copy link
Collaborator

rniwa commented Sep 20, 2016

I agree, changing the semantics of observedAttribute at this stage would be contentious. Google wanted to special case style and other animated attributes for performance reasons and Mozilla didn't want to special case a single attribute. And then there was an issue of attributeChangedCallback being spammy when there is a lot of attributes or some attribute you don't care keep changing its value. So the compromise was made to always specify a list of attribute to observe.

Defining the list of standard attributes is hard because the list is not finalized. As more attributes are added to the standard, components which relied that those attributes to be not standard will fail to observe the attribute value changes even if the attributes only had a meaning in some builtin elements. As such, this approach poses a serious forward compatibility concern.

@WebReflection
Copy link

I think having observedAttributes either an Array or a RegExp would solve pretty much every issue raised in here. "catch-all" would be as easy as /.*/ and it's possible to filter out stuff like /^(?!data-)/ or /^(?!style$)/ to rule out style only.

If the attribute name matches the RegExp then the method is triggered.

The current Array could be also represented as RegExp:

["my-attr", "my-other"]
/^(?:my-attr|my-other)$/

For performance concerns, if any faster, developers can still use the Array and let browsers optimise it as possible.

My 2 cents

@rniwa
Copy link
Collaborator

rniwa commented Sep 20, 2016

I don't think we want to be evaluating regular expressions against attribute names whenever its value changes. That would be unacceptably expensive.

@WebReflection
Copy link

WebReflection commented Sep 20, 2016

Agreed, and I wasn't suggesting that. Implementors can whitelist attributes by name once, instead of each time, so that you'd evaluate only first time it matches the RegExp and from that time on use the list.

In better words: the RegExp filters once the list of attributes.
It'd be like "JIT" so a bit slower, if a RegExp is used instead of an Array, but "fast as" after first test.

On specs level it should probably be simpler, but AFAIK specs don't care much about optimisation details.

@rniwa
Copy link
Collaborator

rniwa commented Sep 20, 2016

I'm not sure what you mean by whitelist. Basically, anytime an attribute is added, we have to evaluate the regular expression. The alternative is to cache the list of attributes we've matched in the past, which can increase the memory footprint. I don't think either approach is acceptable.

@WebReflection
Copy link

Basically, anytime an attribute is added, we have to evaluate the regular expression

Only the first time. Meta-speaking

if (!_whiteList.contains(newAttribute)) {
  if (!_blackList.contains(newAttribute)) {
    if (observedAttributes.test(newAttribute)) {
      _whiteList.push(newAttribute);
    } else {
      _blackList.push(newAttribute);
    }
  }
}

After the first time, you'll do that also only if the name is unknown/different which is usually not common.

Of course performance would be penalised compared to just first check but like I've said, we could keep the current Array when performance are a concern.

Mine was just an idea, it's surely more work for vendors and specs author, but it'd probably make developers happy giving them the ability to intercept whatever they want if/when they want.

I personally wouldn't mind keeping as it is, but I understand other developers needs.

@Mr0grog
Copy link

Mr0grog commented Sep 20, 2016

As a web developer, @WebReflection's idea seems like a pretty neat and useful way to split the difference between performance and flexibility here.

I definitely appreciate that using an expression of some sort (whether a regex or something else) certainly has a little overhead, but am curious as to whether the memory cost of caching the results is really that high—since the list is specific to the custom element type, not the instance, it doesn’t seem like that’d be a really huge cost. And wouldn’t the cache effectively be the same as just prepending a matched attribute name to the list anyway? Would it be any more expensive (memory-wise) than an author providing a list of 10 strings rather than a list of two expressions? (I do get than this means using mutable, non-fixed length structures, but I have to imagine there are lots of opportunities for smart optimizations, too.)

And if we can expect people to use a similar set of attributes on most instances of an element (again, this seems like a reasonable assumption to me), wouldn’t we be likely to arrive at a relatively fixed list of strings that pretty much always matches before falling back to one of the provided expressions pretty quickly?

@trusktr
Copy link
Author

trusktr commented Sep 21, 2016

I think having observedAttributes either an Array or a RegExp would solve pretty much every issue raised in here. "catch-all" would be as easy as /.*/ and it's possible to filter out stuff like /^(?!data-)/ or /^(?!style$)/ to rule out style only.

With caching, I think this could be useful, and adding it to spec later would remain backwards-compatible. It would also be opt-in, so if an array is provided then no extra memory footprint for the cache list.

Although I like the idea of using RegExp for observedAttributes, extending a super class's observedAttributes then becomes tricky (imagine super observedAttributes is a list, and the child class wants to have a RegExp).

Would it be any more expensive (memory-wise) than an author providing a list of 10 strings rather than a list of two expressions?

The phrase "list of two expressions" makes me think: maybe observedAttributes can be a list as now, but can also contain RegExps as well (with caching as mentioned). In this case, extending super isn't so bad.

Defining the list of standard attributes is hard because the list is not finalized. As more attributes are added to the standard, components which relied that those attributes to be not standard will fail to observe the attribute value changes even if the attributes only had a meaning in some builtin elements. As such, this approach poses a serious forward compatibility concern.

From a design-pattern perspective, would it be more web-manifesto-friendly for "global" attributes to be observedAttributes on a base class that all elements extend from, like HTMLElement? That would be a breaking change after v1, but I feel like it makes sense in order for things to be explainable rather than just magic. The downside is that calling super would be required for all custom elements or the custom element would no longer observe things like style:

class MyElement extends HTMLElement {
  static get observedAttributes() {
    return super.observedAttributes().concat(...)
  }
}

@trusktr
Copy link
Author

trusktr commented Sep 21, 2016

After reading your responses on the original topic, I am okay with current behavior. I should also be less nit picky. :}

@WebReflection
Copy link

WebReflection commented Sep 21, 2016

extending a super class's observedAttributes then becomes tricky

if really needed, there are ways.
Following a very basic example

class MyElement extends HTMLElement {
  static get observedAttributes() {
    return new RegExp("^my-el-|" + super.observedAttributes().source);
  }
}

@trusktr
Copy link
Author

trusktr commented Sep 21, 2016

@WebReflection But in that example we're assuming that super.observedAttributes() is a RegExp, but it may also be an array.

@trusktr
Copy link
Author

trusktr commented Sep 21, 2016

So I think requiring observedAttributes to be an array -- possibly containing RegExp instances -- would make assumptions easier. Then the pattern can just always be:.

class MyElement extends HTMLElement {
  static get observedAttributes() {
   return super.observedAttributes().concat(/* your new stuff here */)
  }
}

@WebReflection
Copy link

like I've said, I'm OK with current status and extendability through .concat but you know what you extend so that if super has no super.observedAttributes you'll have an error trying to concat nothing.

In few words, we are making up inexistent problems because reality would be like the following:

  • if super is Array and you want RegExp you can new RegExp("your-thing|" + super.stuff.join('|')) (simplification)
  • if super is Array and you want extend you use .concat
  • if super is RegExp and you want extend either copy super.observables and enrich it without even calling super or use new RegExp
  • if super is RegExp and you want Array, you still new RegExp(['your', 'cases'].join('|') + super.stuff)

Since people will abuse these things and it's easy to get it wrong, I guess we'll stick with current status and that's it, it's like good old Object.prototype.watch where either you know upfront or you cannot spy on everything like a proxy would do.

@Mr0grog
Copy link

Mr0grog commented Sep 22, 2016

FWIW, I don’t feel particularly concerned about the extendability issue—I think the status quo for extension is fine. That said, I think making observedAttributes always a list (of strings and/or expressions) would be better than a list-of-strings-or-one-expression for two reasons:

  • Extending an existing regular expression isn’t necessarily simple. For example, what if a super class uses u flag on the expression but you don’t want to on the subclass or vice-versa? Adding another expression onto a list avoids subtle errors people could have here.
  • Extension in general, even if just adding more strings, is straightforward. Instead of handling four possible cases as above, there’s only one: concat() onto the list.

I think supporting some kind of expression would also be good in so far as it “explains the platform” for data-* attributes / the dataset property, which I don’t think could be handled through custom elements as-is right now (please correct me if I’m wrong on that).

@treshugart
Copy link

I think if we wanted to maximise potential, making it a callback that returns true if it is to be observed, might be a good idea.

@rniwa
Copy link
Collaborator

rniwa commented Sep 22, 2016

We definitely don't want to turn this into a callback. The whole point of this property is to avoid invoking JS whenever possible in the engine code.

@allenhwkim
Copy link

For my case, I ended up with building my own instead of using observedAttributes and attributeChangedCallback() because;

  1. I wanted to observe all attribute changes instead of few.
  2. and, the list of attributes to observe are not limited, but dynamic. (e.g. google maps attribute.)

What I do is to simply call this from connectedCallback

function observeAttrChange(el, callback) {
  var observer = new MutationObserver(mutations => {
    mutations.forEach(mutation => {
      if (mutation.type === 'attributes') {
        let newVal = mutation.target.getAttribute(mutation.attributeName);
        callback(mutation.attributeName, newVal);
      }
    });
  });
  observer.observe(el, {attributes: true});
  return observer;
}
connectedCallback() {
    observeAttrChange(this, ....);
}

@trusktr
Copy link
Author

trusktr commented Nov 19, 2017

@rniwa, your previous thought of avoiding invoking JS is not really solving performance. Instead of the end user's conditional statements running logic for certain attributes, we've merely moved that logic into the HTML engine, so now the HTML engine has to conditionally check observedAttributes in order to fire the end user's attributeChangedCallback. This practically does nothing to improve performance, because regardless of whether the HTML engine checks observedAttributes or not, the user will still have some code like follows:

attributeChangedCallback (a, o, n) {
  if (a == 'foo') { ... }
  else if (a == 'bar') { ... }
  else if (a == 'baz') { ... }
  // etc...
}

So the only thing being saved is some extra no-op function calls that will do nothing because most people will already be checking conditionally for certain attributes.

Is there really a significant gain from observedAttributes that outweighs the burden of having to remember to manually list all observed attributes? Are there benchmarks somewhere?

@annevk
Copy link
Collaborator

annevk commented Nov 20, 2017

Yes there is. Going from C++ to JavaScript is expensive.

@rniwa
Copy link
Collaborator

rniwa commented Dec 4, 2017

Yeah, calling out to JS at all is extremely expensive. The fact we have to remember the old attribute value and queue things up when multiple attributes are modified, etc... all add up to something like 3x overhead the last time I measured.

@trusktr
Copy link
Author

trusktr commented Dec 5, 2017

What if someone wants to observe any attributes without knowing what they are in advance? Would you prefer for them to reimplement attribute observation like @allenhwkim had to above?

@rniwa
Copy link
Collaborator

rniwa commented Dec 5, 2017

What are use cases in which you want to observe changes to arbitrary set of attributes?

@allenhwkim
Copy link

allenhwkim commented Dec 5, 2017

@rniwa Common use cases are building an element that wraps an visual object; map, chart, drawing, etc.

Assuming your are building a <google-map> custom element, and you are accepting all map properties; more than 30 properties, through attributes; e.g., center, zoom, etc, Then, you also want to monitor the attribute change and update the map when it is changed. It would be ideal to handle it dynamically rather than listing 30+ attributes(https://developers.google.com/maps/documentation/javascript/reference#MapOptions).

It's not exactly related to this issue, but I have tried to write a google map on this article. https://medium.com/allenhwkim/back-to-element-c4aecf3c6b64

@rniwa
Copy link
Collaborator

rniwa commented Dec 5, 2017

Assuming your are building a <google-map> custom element, and you are accepting all map properties; more than 30 properties, through attributes; e.g., center, zoom, etc, Then, you also want to monitor the attribute change and update the map when it is changed.

That doesn't involve observing all content attributes. What you need is simply a dynamic filtering of a pre-determined set of content attributes. You can do that using observedAttributes and then applying any filtering mechanism.

@allenhwkim
Copy link

@rniwa Is there a way to use observedAttributes without listing 30+ attributes within the getter?

@rniwa
Copy link
Collaborator

rniwa commented Dec 6, 2017

@rniwa Is there a way to use observedAttributes without listing 30+ attributes within the getter?

No, you should list those attributes. That would be still much cheaper than having to serialize the style attribute whenever the inline style is changed.

@hmans
Copy link

hmans commented Jan 16, 2021

I would like to add an example for a situation where I do not know the attributes my custom elements will expose in advance.

I am writing a library called three-elements that, at import time, walks through the exports of Three.js and dynamically defines a new custom element for every one that can be instantiated. When it encounters an attribute that also exists as a property on the wrapped Three.js object, setting this attribute will also set that object's property.

Unless I'm missing something terribly obvious, there is no way to implement this using observedAttributes -- maybe except for hand-crafting each element, or keeping a list of Three.js object properties somewhere, but this library aims at working with any version of Three.js -- which is a big selling point for places that, for example, have their own forks of the library.

I've solved this by using the MutationObserver trick described above, and it generally works well, with two caveats:

  • I don't know (yet) what the performance impact of this will look like
  • Writing to an attribute will only reflect the written value to the wrapped object's property once the MutationObserver invokes its callback, which creates some (slightly) confusing behavior in some situations (for example, in my tests for this assignment logic, I need to run my assertions in a setTimeout callback because otherwise the new value would not have been forwarded yet.)

I understand why things are the way there are and wouldn't even want to go anywhere near suggesting a change -- I merely wanted to present an (admittedly unusual) use case in the hopes that maybe new patterns will emerge in the future.

(For example, it would rock to be able to imperatively update the observableAttributes list at runtime -- I could then do this from within my constructor code once it actually has an instance of the wrapped class whose properties it could then walk and make a list of.)

(Or, alternatively, letting me tell the mechanism to yes, please run the attributeChangeCallback for any attribute change. :b)

Thanks & stay healthy!

@justinfagnani
Copy link
Contributor

Note that @hmans case isn't that unusual. A-Frame hit the same issue with their Entity-Component-System, and it kept them on the v0 specs for a long time. I think they might solve this by patching setAttribute and reading all attributes at connected, which obviously has some problems. Maybe they use a MutationObserver too.

I remember in the very early group meetings that Elliott Sprehn proposed some kind of wildcard with a sigil that would exclude style and class. My recollection is that the idea wasn't shot down, just deferred until later if a need came up.

btw, @hmans a solution that works with observedAttributes would require some kind of runtime or build-time introspection on the type of object being wrapped. If class fields were present on the prototype in some way, you'd be able to do something, but instead maybe you can introspect the source.

@hmans
Copy link

hmans commented Jan 16, 2021

btw, @hmans a solution that works with observedAttributes would require some kind of runtime or build-time introspection on the type of object being wrapped. If class fields were present on the prototype in some way, you'd be able to do something, but instead maybe you can introspect the source.

Yeah, I've been thinking (a lot) about approaches like that, but with the Three.js, it's going to prove difficult. The nicest thing I can think of right now is to process its .d.ts files and generate something from them, but that would bind my library to a specific version of Three.js, and that's something I've been wanting to avoid.

There's another crazy idea where I have every element (eg. <three-mesh>) immediately replace itself with a different one (eg. <three-smart-mesh>) that is backed by a class that is created on the fly and outfitted with an observableAttributes filled with the known properties of the object <three-mesh> instantiated, but my library's approachability would probably suffer severely.

Someone's gonna come up with something. :b Thanks for getting back!

@Matsuuu
Copy link

Matsuuu commented Jan 19, 2021

This is something I have been looking for for quite some time already. When developing Web Components, there are plenty of use cases where you want to update the list of observed attributes in your Component.

By providing a simple API, something along the lines of

customElements.updateObervableAttributes('my-component')

or built into the HTMLElement class,

// inside HTMLElement
this.updateObservableAttributes();

The developer could manage the updating of the attributes themselves.

@WebReflection
Copy link

@Matsuuu the registry is public, meaning every script could update your observed attributes ... in a world first-come, first-serve that idea is as bad as allowing prototype changes after Custom Elements definition.

In these cases, using a MutationObserver for attributes doesn't seem like bad at all, so the primitive is there, we just need to use it when appropriate.

@hmans
Copy link

hmans commented Jan 26, 2021

@justinfagnani

I think they might solve this by patching setAttribute and reading all attributes at connected, which obviously has some problems.

I'm about to do this in three-elements, too, and it feels like a pretty solid solution, especially since, eventually, I can make this smart enough to make observedAttributes co-exist with a separate at-runtime mechanism that can make decisions on whether the attribute exists or not.

What were the obvious problems you were referring to here?

@hmans
Copy link

hmans commented Jan 26, 2021

I've found one drawback: while the overloaded setAttribute will correctly trigger when attributes are set by eg. frameworks mutating the DOM, it will not be triggered when the user modifies attributes values in the inspector.

Are there any others?

@WebReflection
Copy link

it will not be triggered when the user modifies attributes values in the inspector.

weird use case though, but once again, MutationObserver has been created to solve exactly this issue.

@hmans
Copy link

hmans commented Jan 26, 2021

weird use case though

Absolutely, and certainly not critically important in any way. At the moment I'm enjoying that this is something that works because it helps me sell the library ("check it out, you can just change values in the DOM!") - but it is something that I would be happy to give up if (if) the setAttribute approach proves significantly faster than using MutationObservers. Up to this point I don't really have the data to back one or the other.

@hmans
Copy link

hmans commented Feb 5, 2021

A quick update from my side on how I've been tackling this with three-elements:

After using MutationObserver for a while, I eventually removed it, and instead went with overloading setAttribute in the base class for all the elements my library is defining. This is working great with all of the major intended use cases for the library; the only thing that no longer works with this is reacting to manual changes made to the DOM through the browser's devtools.

I've had at least one user of the library tell me that this is a feature they really liked and wanted to see return, so I implemented an opt-in MutationObserver mode that also prints a console warning about its performance impact.

All in all, things are plenty good now, but of course if there is a future where I can whitelist all attributes without working around things, I want in. :-) The biggest caveat right now is that there are so many nice web component authoring libraries out there that I would theoretically like to implement in my library, but only few of them allow me to hook into their underlying classes so I can apply my setAttribute trickery.

@Danny-Engelman
Copy link

After using MutationObserver for a while, I eventually removed it,

Why did you remove it and went for the setAttribute hack?

@hmans
Copy link

hmans commented Feb 7, 2021

Why did you remove it and went for the setAttribute hack?

For context, it is important to understand that three-elements is, at its core, a game engine. Not a terribly good one, but it does the typical things a game engine does: provide structure, a solid ticker, and have a focus on performance. In games, doing everything within 16.7ms is critically important; in games on the web, where you're often dealing with lower-powered devices and/or want to conserve their batteries, even more so.

MutationObserver was posing two problems for me:

  1. It was relatively expensive in terms of performance. When the project only has a couple of elements that get their attributes mutated on every frame, it wouldn't be a big problem (besides of using more CPU cycles than the alternative), but adding a higher number of elements -- maybe 50 and up -- to the scene quickly brought down FPS (by making individual ticks need more than said 16.7ms.)

  2. Unless I'm mistaken, it performs its work asynchronously, which of course is totally understandable due to what it does and how it works, but it did create some confusing moments where some code updated another element's attribute, and the change wouldn't actually have registered until the next JS tick (not rAF tick), requiring me to go though queueMicrotask or setTimeout quite often. Not a terribly big problem, but it didn't particularly improve the developer experience.

The first issue is of course much more critical than the second. Applying the setAttribute hack improved performance from "you can't build a simple Match-3 game with 60 FPS" to "you can barely find the attribute assignments in the profiler's flame chart", which is a good optimization in the context of what I'm doing here.

Last but not least, I should add that mutating my elements' attributes isn't even the recommended way to work with the library; I typically advise users of my library to interact with the underlying Three.js object instances directly where possible, much like the react-three-fiber team advises their users to prevent React re-renders, but not only do I think it's great that you can also set attributes without incurring a significant performance cost; ironically it is me who is using three-elements in a way where I have to change elements' attributes and can't work with the wrapped objects directly (I'm working with an experimental stack that involves server-sent and -diffed HTML.)

I hope all of this sort of makes sense. Please let me know if you have questions, and even more so please let me know if you believe I had simply been using MutationObserver (or anything else, for that matter) incorrectly.

Thanks!
H.

@Buderdene
Copy link

I think if we wanted to maximise potential, making it a callback that returns true if it is to be observed, might be a good idea.

21132

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

No branches or pull requests