what's wrong with:

super.connectedCallback();

? 🤔

you check for !window.customElements without checking if window exists at all, so this will never pass through the ||, or better, code will fail anyway if window is not around. I suggest to either remove that || global, as never reached, and maybe use self instead of window for Worker cases, or even better: globalThis.

That being said, you could use the proper, maintained, polyfill instead.

FYI LinkeDOM has customElements too

We can use simply this.doConnectedCallback() here right?

Does this thing work?
From my understanding, this.sceneEl is defined when executing ANode connectedCallback() but you call it after here.

I confirm, I just added a console.log, this.sceneEl is always undefined here. If you put super.connectedCallback(); above, it will contain the scene element.

Same here, you wanted to write this.doConnectedCallback.bind(this) right?

simply super.connectedCallback()

I don't understand why you created a doConnectedCallback function if you don't use it here. The second time connectedCallback is called, we know the condition is false, so you intended do use this.doConnectedCallback.bind(this) here to not evaluate the condition again, right?

It's effectively the same but this way when read it's more clear to see that we're just deferring the call. Don't have to read understand doConnectedCallback

evaluating the condition only happens once so perf-wise is negligible over more clear code

In your logic, if it's more clear to you to see that we're just deferring the call, and I can agree with that, then you don't even need to create a separate function doConnectedCallback, you can just put the code inside connectedCallback after the condition, unless I'm missing something why you created it. That's why it bothered me reading this code, when I have this pattern I usually create a function like this to use it as the callback, mostly important when the condition is costly so it's not executed a second time. Here I agree the condition cost is negligible.

To me, the condition is evaluated twice when the readyState is loading:

• connectedCallback called
• if (document.readyState === 'loading') evaluated to true
• document.addEventListener('DOMContentLoaded', this.connectedCallback.bind(this)); callback registered
• connectedCallback called because of the DOMContentLoaded event
• if (document.readyState === 'loading') evaluated to false
• doConnectedCallback called

in comparison, using doConnectedCallback as the callback:

• connectedCallback called
• if (document.readyState === 'loading') evaluated to true
• document.addEventListener('DOMContentLoaded', this.doConnectedCallback.bind(this)); callback registered
• doConnectedCallback called because of the DOMContentLoaded event

I created it for readability. I like small focused functions when possible

even if evaluated twice cost is negligible and it happens before the scene loads. Dynamically added entities once the DOM has loaded won't defer the call.

use doConnectedCallback here

simply super.disconnectedCallback();

This line should really be at the top.

@dmarcos there is actually no issue here, but a rather confusing code. I actually didn't see you do this.sceneEl twice here. You do this here:

var sceneEl = this.sceneEl; // but we know it's actually undefined because we didn't call super.connectedCallback()
...
super.connectedCallback();
...
sceneEl = this.sceneEl;

so I'd suggest just to use

var sceneEl;
...
super.connectedCallback();
...
sceneEl = this.sceneEl;

or

super.connectedCallback();
var sceneEl = this.sceneEl;

I see you chose the first suggestion, great.

We didn't have this call before. We call it a second time below.

I tested the previous code, putting a console.log in detachedCallback and removing an a-entity element via the chrome inspector, it was called twice, I'm not sure why twice.
Here in the new code, this is called four times. Removing this line, disconnectedCallback is called twice, no error, tests are all passing.

Thanks. I removed the duplicated call but don't see disconnectedCallback called twice. Can you double check or share steps to reproduce? Thanks

You can add a console.log exactly where you just removed the line, open the inspector, right click on the node and remove. Anyway it's the same behavior than in the previous version now.

Ah I just understood why I have it twice, I was removing a-entity that has a child a-camera, so I'm removing two entities at the same time that's why.

simply super.disconnectedCallback();

This is similar to what we had in the polyfill https://github.com/dmarcos/document-register-element/blob/cd665644367686d5645048f853d22fef9584bbf0/src/document-register-element.js#L529-L560 right? Previously we had a single MutationObserver instance for the whole document, here we have a MutationObserver instance for each node. Do we have to worry about performance here?

I'm not too familiar about how the old polyfill worked. Not sure about the overhead of an observer. Any ideas on how to do it with one are welcome. it's a bit unfortunate that customElements V1 observed attributes can only be declared statically.

I'm not familiar with it either, it was actually the first time I saw a MutationObserver object myself. Not sure about the performance really, I was just reading the comment about perf above the code I linked, this comment https://github.com/dmarcos/document-register-element/blob/cd665644367686d5645048f853d22fef9584bbf0/src/document-register-element.js#L526-L528

if you check WebKit source code around MO you realize having one or many doesn't make much of a difference.

Thanks @WebReflection for checking that. So we're good then.

thanks @vincentfretin and @WebReflection for the feedback

Note that in previous code, the super method was called before setting the default components. This super method should be called at the top, an example super.connectedCallback(); is initializing (currently wrongly) this.sceneEl, and the component init may use this.sceneEl but it's probably not the case here.

@WebReflection with this statement the polyfill is a pass-through if custom elements API is defined? Just following the how-to: https://github.com/ungap/custom-elements#how-to

Thanks for the feedback

the poly includes features detection and leave untouched browsers that implement latest status of the specs:

• custom elements builtin extends (only safari / webkit gets fixed)
• customElements.whenDefined('some-comp').then(Class => { ... }) is patched for older browsers that don't pass the related class to the promise once the definition is registered

Thanks @WebReflection. Just to double check. We're good just requiring the polyfill, are we? No other checks or initialization necessary?

the poly includes features detection for all cases, including jurassic one where customElements is not a thing ... if you want to rise your target browsers, the Safari/WebKit only is an option but it requires manual feature detection ... happy to help with that though

P.S. if you've fully switched to customElements without builtin extends support, that might even not be needed

This change was wrong, we need to put back the previous line.
I get this error with the change:

a-entity.js:73 Uncaught RangeError: Maximum call stack size exceeded
    at a.doConnectedCallback (a-entity.js:73:3)
    at a.connectedCallback (a-node.js:42:10)
    at a.doConnectedCallback (a-entity.js:75:11)
    at a.connectedCallback (a-node.js:42:10)
    at a.doConnectedCallback (a-entity.js:75:11)
    at a.connectedCallback (a-node.js:42:10)
    at a.doConnectedCallback (a-entity.js:75:11)
    at a.connectedCallback (a-node.js:42:10)
    at a.doConnectedCallback (a-entity.js:75:11)
    at a.connectedCallback (a-node.js:42:10)

For a-scene, a-assets, a-mixin, we can use this.doConnectedCallback(); because we don't inherit from those.
But for a-node and a-entity, you can't use it, because it will call the doConnectedCallback from the derived class, we don't want that here.