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

Custom Element - untrackable upgrade #671

Closed
WebReflection opened this issue Sep 27, 2017 · 26 comments
Closed

Custom Element - untrackable upgrade #671

WebReflection opened this issue Sep 27, 2017 · 26 comments

Comments

@WebReflection
Copy link

AFAIK there is no way to know when an element would be upgraded to a CE, if not using timers or rAF to verify it's instanceof its own defined class.

class TestCase extends HTMLElement {
  static get observedAttributes() { return ['with']; }
  attributeChangedCallback() {
    console.log('%cI have attributes', 'font-weight: bold');
  }
  connectedCallback() {
    console.log('%cI am live', 'font-weight: bold');
  }
}
customElements.define('test-case', TestCase);

const tp = document.createElement('template');
tp.innerHTML = '<test-case with="attribute"></test-case>';

const target = tp.content.firstChild;
const check = mate => console[target instanceof TestCase ? 'info' : 'warn'](mate);

// imagine I'd like to parse `target`
// without compromising its functionality
// and reacting once it gets promoted
// right before connectedCallback or attributeChangedCallback
// basically what V0 createdCallback was doing

check('sync template'); // fails
customElements.whenDefined('test-case').then(() => {
  check('async template');  // fails

  // triggers attribute and conected
  document.body.appendChild(target);

  check('sync live'); // OK
  customElements.whenDefined('test-case').then(() => {
    check('async live');  // OK
    console.log(target.outerHTML);
  });
});

Specially when it comes to interact with templates, something both hyper and lit HTML libraries do, it's fairly impossible to behave properly with parsed attributes and/or trust the prototype.

Is there any event or planned callback that would solve this? The constructor is not good enough and third parts libraries can only know, through the node name and the registry, if the node is a Custom Element, but these libraries have no way to understand when such node will behave like one.

Patching connectedCallback and attributeChangedCallback directly on the node feels wrong and it's a very dirty approach.

Thanks for any sort of clarification/plan/idea/hint .. you name it.

@WebReflection
Copy link
Author

WebReflection commented Sep 27, 2017

After further investigation, it looks like the following is not even possible:

Patching connectedCallback and attributeChangedCallback directly on the node feels wrong and it's a very dirty approach.

Not only those two callbacks are triggered indirectly with the node as context, even monkey-patching the Class prototype, after it's registered, won't make the upgrade observable when a Custom Elements goes from a template to a live content.

In few words there's no work-around if not through timers/rAF/thenable checks.

@WebReflection
Copy link
Author

WebReflection commented Sep 28, 2017

Update

The only way to intercept/interact before attributeChangedCallback or connectedCallback are triggered, is by using DOMNodeInsterted event on the document.

However, these events have been deprecated, but MutationObserver would not be notified before the component upgrade, always after.

check('sync template'); // fails
customElements.whenDefined('test-case').then(() => {
  check('async template');  // fails

  document.addEventListener('DOMNodeInsterted', e => {
    if (e.target === target) console.log('got it before checks');
  });

  // triggers attribute and conected
  document.body.appendChild(target);

  check('sync live'); // OK
  customElements.whenDefined('test-case').then(() => {
    check('async live');  // OK
    console.log(target.outerHTML);
  });
});

Accordingly, there's still no way to intercept a Custom Element upgrade without showing deprecations and/or violations warnings in console.

@WebReflection
Copy link
Author

More context

Just to explain what is the real issue here, consider the following class

customElements.define('direct-case', class extends HTMLElement {
  connectedCallback() { console.log('connected'); }
  get direct() { return this._direct; }
  set direct(value) {
    console.log('this log might never happen');
    this._direct = value;
  }
});

Now, assuming I have two <direct-case> elements, one already on the body, and one in a template.

document.body.innerHTML = '<direct-case>live</direct-case>';
const tp = document.createElement('template');
tp.innerHTML = '<direct-case>limbo</direct-case>';

Now, using the same function, I'd like to set the direct property if the element is known.

function setDirectValue(node, value) {
  const name = node.nodeName.toLowerCase();
  if (name === 'direct-case') {
    customElements.whenDefined(name).then(() => {
      node.direct = value;
    });
  }
}

// perform exact same operation
var rand = Math.random();
setDirectValue(document.body.firstChild, rand);
setDirectValue(tp.content.firstChild, rand);

You might notice that the log this log might never happen happens actually only once, and it's for the live <direct-case> node.

What's going on with the node in a limbo?

Well, accessing tp.content.firstChild.direct will show the random value ... it seems that after all everything is fine, right?

It's not. That is an own property (expando) on the node and it will shadow its prototype behavior once the node gets promoted.

// so now we have the node on the document
document.body.appendChild(tp.content);

// and indeed it's now the same of the first node we had (true)
document.body.lastChild instanceof document.body.firstChild.constructor;

// but what happens if we set the property again?
document.body.lastChild.direct = Math.random();

// no console log whatsoever ... let's try again with the other node
document.body.firstChild.direct = Math.random();

// "this log might never happen"
// Yeah, the log is shown, everything is fine !!!

As summary, without a mechanism to intercept upgrades Custom Elements are potentially doomed for third parts libraries.

We can retrieve their class through their node name but we cannot understand if these are fully functional and, in case these are not, using an instanceof operation or any other alternative will not provide a mechanism to interact with elements once upgraded.

However, using DOMNodeInsterted seems to trigger right before the upgrade mechanism, so that the following hack seems to be the only way to deal with the current situation.

function setDirectValue(node, value) {
  const name = node.nodeName.toLowerCase();
  if (name === 'direct-case') {
    customElements.whenDefined(name).then(() => {
      const Class = customElements.get(name);
      if (node instanceof Class) {
        node.direct = value;
      } else {
        document.addEventListener(
          'DOMNodeInserted',
          function upgrade(e) {
            if (e.target === node) {
              document.removeEventListener(e.type, upgrade);
              // here node is still not instanceof Class
              // however, a then() / tick will be enough
              // let's use the logic already in place
              setDirectValue(node, value);
            }
          }
        );
      }
    });
  }
}

Using above function instead of the previous one, will ensure a correct functionality for the Custom Element and its future prototype definition.

I wouldn't even mind going this way, but using DOMNodeInserted produces the following warning in console which is very developer "_un_friendly"

screenshot from 2017-09-28 15-26-42

Outstanding issues

  • even if DOMNodeInserted triggers before the upgrade, it's impossible to interact with the node, as Custom Element, before the connected or attributechangedcallback happens
  • since previous point is true, we could use MutationObserver ignoring its delayed execution 'cause it's going to be too late in any case
  • there is regardless no way third parts libraries can reliably deal with an upgrade from the DOM. Having this exposed through Custom Elements as event or callback would be the ideal solution.

Thanks for your help and patience in reading this through, I hope somebody will chime in at some point and provide some feedback.

Best Regards

@robdodson
Copy link

robdodson commented Sep 29, 2017

The snippet I use to work around this as a custom element author is:

connectedCallback() {
  ...
  this._upgradeProperty('checked'); // assuming my element has a .checked property
}

_upgradeProperty(prop) {
  if (this.hasOwnProperty(prop)) {
    let value = this[prop]; // copy the value from the instance
    delete this[prop]; // delete the shadowing property
    this[prop] = value; // trigger the setter with the copied value
  }
}

Documented a bit more here under the heading "Make properties lazy".

I agree this is a tricky gotcha and we ran headlong into it while working on howto-components.

@rniwa
Copy link
Collaborator

rniwa commented Sep 29, 2017

So the problem here is that there's no way to know when an element gets inserted into the document and upgraded after the matching custom element had already been defined?

So the order of events that lead to a problematic situation is:

  1. A custom element E is defined in some document D
  2. An upgrade candidate C is created for E in a template, a different document, etc...
  3. E is inserted into D
  4. E is upgraded

And you'd like to know when 4 happens? Is it sufficient to know when a particular element gets upgraded? Or do you want to observe all elements that get upgraded to some custom element?

That is, is it sufficient to, let's say have a variant of whenDefined(element) which takes an element and resolves a promise when the element gets upgraded, or you want some kind of observer which gets involved whenever some element gets upgraded to a custom element of a given name. e.g. setCustomElementUpgradeCallback('my-element', (element) => /*do stuff with element*/).

@WebReflection
Copy link
Author

Is it sufficient to know when a particular element gets upgraded?

yes, but from the outside, not from within the class or it's still unobservable.

Or do you want to observe all elements that get upgraded to some custom element?

I want to know, if a node is a custom element, when it will get upgraded.

is it sufficient to, let's say have a variant of whenDefined(element) which takes an element and resolves a promise when the element gets upgraded,

That would be just perfect. The simplest, the better.

In this case, my only concern is what happens if a node is not a custom element.
If the node is not an HTMLUnknownElement and the promise is resolved right away, let's say in cases such whenDefined(document.documentElement), then I'm more than OK with this proposal/solution.

It solves from the inside, it solves from the outside, everybody wins.

@rniwa
Copy link
Collaborator

rniwa commented Sep 29, 2017

Yeah, basically if the element is HTMLUnknownElement, it would reject the promise instead of resolving it later when it gets upgraded. I think this is a fairly simple addition to the API.

@WebReflection
Copy link
Author

WebReflection commented Sep 29, 2017

it would reject the promise instead of resolving it later when it gets upgraded.

just to be sure I understand ... considering the following code:

customElements.define('be-fore', class extends HTMLElement {});

const tp = document.createElement('template');
tp.innerHTML = '<be-fore></be-fore><af-ter></af-ter>';

// are you saying this will resolve ...
customElements.whenDefined(tp.content.firstChild).then(console.log);

// ... but this will throw right away ?
customElements.whenDefined(tp.content.lastChild).catch(console.error);

document.body.appendChild(tp.content);

// since the element is defined after ?
customElements.define('af-ter', class extends HTMLElement {});

This is a caveat with CE that might work for me, but it's hard to explain together with the current whenDefined mechanism, where you ask for a CE not defined yet.

To solve this though, the following might work as well:

function setDirectValue(node, value) {
  customElements
    .whenDefined(node.nodeName.toLowerCase())
    .then(() => {
      customElements.whenDefined(node)
        .then(() => {
          node.direct = value;
        });
    });
}

This works for me.

@rniwa
Copy link
Collaborator

rniwa commented Sep 29, 2017

No, customElements.whenDefined(tp.content.lastChild).catch(console.error); would never resolve or reject or throw because af-ter is a valid custom element for which its definition might come in later asynchronously.

I'm suggesting that if you passing a div element, an element without -, etc... that can't possibly a custom element, we would return a rejected promise.

@justinfagnani
Copy link
Contributor

The problem of pre-upgrade properties can be addressed from two different places: outside the element, where the properties are set, or inside the element when it boots up.

I think the problem with offering APIs like whenDefined(element) or something else that lets the setter wait until the element is upgraded to set properties is that this waiting would need to be implemented in any framework that wants to handle custom elements upgrades - they'll need to do work and opt-in to this behaviors.

If we help custom elements themselves consume pre-upgrade properties, then frameworks won't need to do anything, and custom elements will be upgradable in more cases.

Would it be possible to add something to upgrading like:

  1. Remove and store all own properties on the element
  2. Perform existing upgrade
  3. Re-set the properties from (1)

If that's too drastic, could there be a static property similar to observedAttributes that opts into the properties that need to be removed and re-set? CE base-classes/frameworks could implement a getter that returns the properties that it generates accessors for, or use the property to know what accessors to generate.

@WebReflection
Copy link
Author

CE base-classes/frameworks could implement a getter that returns the properties that it generates accessors for

I don't think I like the idea of another observedAttributes like static accessor, specially because we're mostly talking about prototype accessors and inheritance done right with accessors is not as trivial as it looks like.

I've rarely seen components following this pattern for attributes:

class B extends A {
  static get observedAttributes() {
    // Set used to enable overwrites
    return new Set(super.observedAttributes.concat(['b', 'c']));
  }
}

I strongly doubt accessors will be handled as such:

class B extends A {
  static get observedAccessors() {
    const p = super.prototype;
    const accessors = function (k) { return !('value' in Object.getOwnProperty(this, k)); };
    return new Set(Reflect.ownKeys(p).filter(accessors, p)
        .concat(Reflect.ownKeys(this.prototype).filter(accessors, this)));
  }
}

which is more convoluted than .whenDefined ... yet I have the feeling this Custom Elements part will never be too welcomed; it feels like CE are just second-class citizens on the DOM.

@trusktr
Copy link
Contributor

trusktr commented Oct 8, 2017

whenUpgraded(element) is a better name so it isn't confusing with whenDefined("string-name").

@rniwa
Copy link
Collaborator

rniwa commented Oct 10, 2017

It's kind of too late but this is one of the reasons we were opposed to adding upgrades and only wanted synchronous definitions of custom elements in the first place. This exact conversation came up during the internals discussions we had at Apple, and we came up with many convoluted solutions and we disliked them all.

Would it be possible to add something to upgrading like:

  1. Remove and store all own properties on the element
  2. Perform existing upgrade
  3. Re-set the properties from (1)

If that's too drastic, could there be a static property similar to observedAttributes that opts into the properties that need to be removed and re-set? CE base-classes/frameworks could implement a getter that returns the properties that it generates accessors for, or use the property to know what accessors to generate.

Alright before deciding whether we like this idea or not, I have a couple of questions to ask.

In step 1, what happens when that property is configurable? Or if Object.freeze had been called on the element instance?

In step 3, are we using the abstract operation OrdinarySet? That abstraction operation has a bunch of side effect like it can create a new own property, etc... Or are we extracting step 3 of that abstraction where we walk up the prototype chain and call Set?

Also, what happens if the author defined an own property which has the same name as some setter on HTMLElement and its prototype chain hierarchy? Since author could add a new property on any prototype object, these properties may end up walking up prototype chain but since any such a walk can be intercepted by a proxy and overridden, this whole process can get quite complicated.

@WebReflection
Copy link
Author

WebReflection commented Oct 10, 2017

I'm pretty sure to obtain expected result the hypothetical upgrade should be performed as such:

  1. const kv = Object.keys(el).map(k => { const v = el[k]; delete el[k]; return [k, v]; });
  2. Perform existing upgrade
  3. kv.forEach(p => el[p[0]] = p[1]);

This operation will automatically involve the prototype chain when it comes to setters + frozen own properties won't have any side effect, simply silently ignored.

There is still an outstanding question for me: during that third point the element would still be in a not fully updated state so that involved setters in the process might be mislead.

How about we keep it simple and we focus just on the whenDefined(element).then... bit ?

If we had that, all the primitives to do whatever we want would be available.

We can detect if a node is an upgraded * custom element already and, if not, we can do on user-land the manual drop things, wait for it, add things back procedure.

Having it simple would also make adoption, polyfill, and cross browser portability easier.

  • to see if a node is an upgraded custom element
const isCustomElement = el => {
  const name = el.nodeName.toLowerCase();
  if (/^([a-z][\S]*?-[\S]*)$/.test(name)) {
    const Class = customElements.get(RegExp.$1);
    if (Class) return el instanceof Class;
  }
  return false;
};

@trusktr
Copy link
Contributor

trusktr commented Oct 10, 2017

Seems like whenDefined just resolving when all need-to-be-upgraded elements have been upgraded would prevent us prematurely setting properties. This would need to include all instances of an element, even one in a template or otherwise, being upgraded without being in a document (unlike spec).

This would be okay, because if we were to make the element with new SomeElement form, then stick it into a template or anywhere not a document, it would be an instanceof the actual custom element.

So upgrading all elements not matter what is fine, to keep consistency, because other wise we have a combination of instances that are a custom element and some that aren't, even if all these instances haven't been put into a document yet.

It would be great if we could rely on whenDefined this way (so all existing instances are guaranteed to have been upgraded no matter where they are). To me, this seems more desirable for reliable programs than preventing elements from being upgraded until being in a document.

Does anyone prefer something from lazy upgrade that they'd rather not lose in order to have whenDefined be more reliable? So far my use cases lean (100%) towards having a reliable whenDefined.

(Note, if we upgrade elements in templates, still we should not call connectedCallbacks or attributeChangedCallbacks until they are in a document)

@WebReflection
Copy link
Author

WebReflection commented Oct 10, 2017

Seems like whenDefined just resolving when all need-to-be-upgraded elements have been upgraded would prevent us prematurely setting properties.

not at all. You can set properties to any DOM node, regardless it will be a Custom Element or not, which is the reason .whenDefined became handy, to interact, or to create, those kind of nodes once the class is registered, which could, or actually could never, happen.

From a hyper/lit~HTML perspective, we parse a template content as soon as injected and without any guarantees of Custom Elements definitions.

We can just guess via element nodeName sniffing and nothing else (pretty much what browsers can do until there is a definition), if an element might be a CE candidate or not, and WeakMap is the only savior here.

Moreover, some element might have observable attributes that would involve prototype setters once invoked, and if such invocation should happen only once the element is live, then let it be like that, our code wouldn't care less and logic will move once live, instead of always.

However, for non DOM related use cases, something I believe nobody here is interested in hearing, having a whenDefined that upgrades before everything live and resolve once everything live is upgraded, granting that from that time on every Custom Element ever would be synchronously available even in templates, the whenDefined calling not at class definition but at live nodes upgrade complete, granting synchronous upgrades from that time on, might be a valuable solution.

Although, what @rniwa said didn't sound promising:

we came up with many convoluted solutions and we disliked them all

@trusktr
Copy link
Contributor

trusktr commented Oct 10, 2017

not at all. You can set properties to any DOM node,

Right, but I meant that we at least need an official proper way to set properties, and as I understand it based on the above examples you wrote, relying on a single call to whenDefined is not currently the proper way to do it, but rather having to add the additional hacks inside of the outer whenDefined is required as the currently proper solution.

Basically, what I'm saying is, your example should just work:

check('sync template'); // fails
customElements.whenDefined('test-case').then(() => {
  check('async template');  // OK
})

This would lead to programs that are more reliable. The downside is that all "inert" template elements will be upgraded.

But so?

Most people define and upgrade all elements right when the page loads anyways.

Perhaps not upgrading template elements is a premature optimization. Furthermore, if

template.appendChild(document.createElement('test-case'))

doesn't create an instanceof TestCase but

template.appendChild(new TestCase)

does, then we have an inconsistent and possibly-confusing API leading to different behaviors in different cases.

@trusktr
Copy link
Contributor

trusktr commented Oct 10, 2017

These sorts of things increase the surface area for bugs without much practical benefit, which is what we don't want.

@WebReflection
Copy link
Author

3 months, no updates. Should I close this issue ?

@rniwa
Copy link
Collaborator

rniwa commented Jan 25, 2018

We can proceed with a concrete proposal if what you want is something like what I outlined in #671 (comment)

Removing & setting all JS properties at the point of upgrades is out of question. We might as well as just close the issue if that's what you want.

@WebReflection
Copy link
Author

I've personally found a work around for this issue that is not too bad, and every few weeks I run through my opened issues to be sure none of them is obsolete, forgotten, or stuck/useless.

I am OK closing this, since I've opened this in the first place, unless others do want to go for something like you outlined in #671.

@trusktr
Copy link
Contributor

trusktr commented Jan 27, 2018

@rniwa I would love something like whenDefined(element), as right now I use a hack to deferr to a future tick. It would just be clean to have an official mechanism, to be sure the code is solid and there's no race conditions that I may have not thought about.

@trusktr
Copy link
Contributor

trusktr commented Jan 28, 2018

whenDefined(element) will only work if one knows the name of the type of element it will be used on in order to get references to those elements, otherwise it will lead to the problem of leaking promises.

To summarize from there:

To paint a small picture of the problem:

Suppose a lib has a custom element that needs to detect if certain child elements are instanceof ACertainClass regardless of what name the end user of the library assigned the constructor to.

The custom element would have to call whenDefined(elementName) of every child's name just to detect if only some of them are instances of a constructor of interest.

So, if an application has thousands of nodes, this can lead to many Promises leaked inside of customElements which the HTML engine is waiting to resolve() but will never resolve them.

This problem still applies with @rniwa's whenDefined(elementInstance) idea.

@annevk
Copy link
Collaborator

annevk commented Feb 18, 2018

Would having #566 solve this? So you can go from an instance to a name for whenDefined() instead of having to overload whenDefined()?

@trusktr
Copy link
Contributor

trusktr commented Feb 22, 2018

I like .whenDefined(SomeElementCtor) more, because that can be applied on a single class rather than on many elements (at least for my cases), so one dangling Promise instead of many in the worst case.

@annevk
Copy link
Collaborator

annevk commented Feb 22, 2018

That is the proposal in #566.

@annevk annevk closed this as completed Feb 22, 2018
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

6 participants