Update A-Frame to custom elements v1 API #5136

dmarcos · 2022-10-13T02:27:30Z

A-Frame is still using custom elements v0 but always running through polyfill since the old API has been deprecated.

This modernizes the code base and makes use of the native APIs shipped in browsers today. Polyfill only kicks in for browser without an implementation of custom elements v1 API.

This will help us to better reason about #5124 and address the root cause.

This PR unifies behavior of all A-Frame defined HTML elements. All connectedCallback associated logic is deferred until DOMContentLoaded fires. This should eliminate the problems with entity inter-dependencies (e.g I want to query an entity from the init method of a component or system that might not have yet been initialized).

arpu · 2022-10-13T08:35:05Z

better use https://github.com/ungap/custom-elements#readme ?

Warning: [@ungap/custom-elements](https://github.com/ungap/custom-elements#readme) is what you are looking for!
on https://www.npmjs.com/package/@webreflection/custom-elements

WebReflection · 2022-10-13T08:43:55Z

src/core/a-mixin.js

+  }
+
+  doConnectedCallback () {
+    ANode.prototype.connectedCallback.call(this);


what's wrong with:

super.connectedCallback();

? 🤔

WebReflection · 2022-10-13T08:49:30Z

src/index.js

+// Custom Elements v1 polyfill
+if (!window.customElements) {
+  const customElementsPolyfill = require('@webreflection/custom-elements');
+  customElementsPolyfill(window || global);


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.

WebReflection · 2022-10-13T08:50:45Z

tests/node/test.js

@@ -11,6 +11,8 @@ suite('node acceptance tests', function () {
    global.navigator = _window.navigator;
    global.document = _window.document;
    global.screen = {};
+    global.HTMLElement = _window.Element;
+    global.window.customElements = {define: function () {}};


FYI LinkeDOM has customElements too

dmarcos · 2022-10-14T08:11:25Z

@kylebakerio Does this PR fix the issue with the camera?

vincentfretin · 2022-10-15T13:12:02Z

src/core/a-entity.js


-      this.addToParent();
+    AEntity.prototype.doConnectedCallback.call(this);


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

vincentfretin · 2022-10-15T13:15:33Z

src/core/a-entity.js

-      if (this.isScene) { return; }
+  doConnectedCallback () {
+    var assetsEl;  // Asset management system element.
+    var sceneEl = this.sceneEl;


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.

vincentfretin · 2022-10-15T13:19:34Z

src/core/a-entity.js

+  connectedCallback () {
+    // Defer if DOM is not ready.
+    if (document.readyState === 'loading') {
+      document.addEventListener('DOMContentLoaded', this.connectedCallback.bind(this));


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

vincentfretin · 2022-10-15T13:23:36Z

src/core/a-assets.js

-        if (this.timeout) { clearTimeout(this.timeout); }
-      }
-    },
+    ANode.prototype.connectedCallback.call(this);


simply super.connectedCallback()

vincentfretin · 2022-10-15T13:24:44Z

src/core/a-assets.js

+  connectedCallback () {
+    // Defer if DOM is not ready.
+    if (document.readyState === 'loading') {
+      document.addEventListener('DOMContentLoaded', this.connectedCallback.bind(this));


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.

vincentfretin · 2022-10-15T13:48:45Z

src/core/scene/a-scene.js

+  connectedCallback () {
+    // Defer if DOM is not ready.
+    if (document.readyState === 'loading') {
+      document.addEventListener('DOMContentLoaded', this.connectedCallback.bind(this));


use doConnectedCallback here

vincentfretin · 2022-10-15T13:49:49Z

src/core/scene/a-scene.js

+  disconnectedCallback () {
+    // Remove from scene index.
+    var sceneIndex = scenes.indexOf(this);
+    super.disconnectedCallback.call(this);


simply super.disconnectedCallback();

vincentfretin · 2022-10-15T14:05:27Z

src/core/a-entity.js

-        return;
-      }
+    // ANode method.
+    super.connectedCallback();


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.

vincentfretin · 2022-10-15T14:07:25Z

src/core/a-entity.js

-      for (componentName in this.components) {
-        this.removeComponent(componentName, false);
-      }
+    super.disconnectedCallback();


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.

vincentfretin · 2022-10-15T14:07:42Z

src/core/a-entity.js

-    }
-  },
+    this.removeFromParent();
+    super.disconnectedCallback.call(this);


simply super.disconnectedCallback();

vincentfretin · 2022-10-15T14:30:31Z

src/core/a-node.js

-        return element;
+    });
+    observer.observe(this, observerConfig);
+  }


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

kylebakerio · 2022-10-16T04:35:12Z

@kylebakerio Does this PR fix the issue with the camera?

Tested it. seems it does.

vincentfretin · 2022-10-16T08:02:53Z

src/core/scene/a-scene.js

+    this.setAttribute('screenshot', '');
+    this.setAttribute('vr-mode-ui', '');
+    this.setAttribute('device-orientation-permission-ui', '');
+    super.connectedCallback();


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.

dmarcos · 2022-10-17T05:52:08Z

@kylebakerio Does this PR fix the issue with the camera?

Tested it. seems it does.

Thanks so much for taking the time to test it. Much appreciated.

dmarcos · 2022-10-19T01:02:20Z

src/index.js

@@ -1,6 +1,8 @@
 // Polyfill `Promise`.
 window.Promise = window.Promise || require('promise-polyfill');

+require('@ungap/custom-elements');
+


@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

vincentfretin · 2022-10-21T10:08:49Z

Two important issues I commented that are not about code style #5136 (comment) and #5136 (comment) are not addressed yet, see my proposed changes here dmarcos#6

vincentfretin · 2022-10-21T11:00:53Z

src/core/a-node.js

@@ -39,7 +39,7 @@ class ANode extends HTMLElement {
      document.addEventListener('DOMContentLoaded', this.connectedCallback.bind(this));
      return;
    }
-    ANode.prototype.doConnectedCallback.call(this);
+    this.doConnectedCallback();


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.

vincentfretin · 2022-10-22T12:18:21Z

I'm currently using a build with my changes in my project. I'll let you know if I find any other issue while I'm developing on it.
Currently it's working great on Firefox and Chrome on Ubuntu, Chrome Android, Safari iPad where I'm testing usually.

dmarcos · 2022-10-24T16:12:20Z

I think this is close to ready. If you give me your blessing I'll go ahead and merge.

vincentfretin · 2022-10-24T16:38:12Z

There is just one last confusing code here https://github.com/aframevr/aframe/pull/5136/files#r1003525392 otherwise good to merge for me.

dmarcos · 2022-10-25T23:45:21Z

All right! Merging. This PR is a scary one 😱 🤞 Perfect for Halloween. Thanks everybody for the feedback. Much much appreciated.

dmarcos mentioned this pull request Oct 13, 2022

Wait for DOMContentLoaded before checking if we have a camera in the DOM #5124

Closed

dmarcos changed the title ~~Updating A-Frame to custom elements v1 API~~ Update A-Frame to custom elements v1 API Oct 13, 2022

vincentfretin mentioned this pull request Oct 13, 2022

Migrate to custom elements v1 API #4167

Closed

4 tasks

WebReflection reviewed Oct 13, 2022

View reviewed changes

vincentfretin mentioned this pull request Oct 14, 2022

User camera not honored anymore and default one injected anyways #3921

Open

vincentfretin reviewed Oct 15, 2022

View reviewed changes

vincentfretin reviewed Oct 16, 2022

View reviewed changes

vincentfretin mentioned this pull request Oct 16, 2022

Review super calls for customElementsV1+ branch dmarcos/aframe#6

Closed

dmarcos commented Oct 19, 2022

View reviewed changes

vincentfretin reviewed Oct 21, 2022

View reviewed changes

dmarcos force-pushed the customElementsV1+ branch from 0c84b75 to cbaf647 Compare October 24, 2022 16:11

Move A-Frame to customElementsV1 API (fix aframevr#4262)

021f972

dmarcos force-pushed the customElementsV1+ branch from 4cd6374 to 021f972 Compare October 25, 2022 23:52

dmarcos merged commit fbfb4de into aframevr:master Oct 25, 2022

vincentfretin mentioned this pull request Jan 6, 2023

Passthrough of attributes on primitives such as a-sphere seems to be broken in 1.4.0 #5203

Closed

kfarr mentioned this pull request Jan 25, 2023

3dstreet asset loader to support a-frame latest 3DStreet/3dstreet#240

Closed

diarmidmackenzie mentioned this pull request Jul 20, 2023

Update linting to enforce ES5 #5218

Closed


		this.addToParent();
		AEntity.prototype.doConnectedCallback.call(this);

Update A-Frame to custom elements v1 API #5136

Update A-Frame to custom elements v1 API #5136

Conversation

dmarcos commented Oct 13, 2022 • edited Loading

arpu commented Oct 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarcos commented Oct 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentfretin Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarcos Oct 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarcos Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

dmarcos Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylebakerio commented Oct 16, 2022

Choose a reason for hiding this comment

dmarcos commented Oct 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentfretin commented Oct 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentfretin commented Oct 22, 2022

dmarcos commented Oct 24, 2022

vincentfretin commented Oct 24, 2022

dmarcos commented Oct 25, 2022

dmarcos commented Oct 13, 2022 •

edited

Loading

vincentfretin Oct 16, 2022 •

edited

Loading

dmarcos Oct 15, 2022 •

edited

Loading

dmarcos Oct 19, 2022 •

edited

Loading

dmarcos Oct 19, 2022 •

edited

Loading