Skip to content

Close a tiny gap in A-Frame loading order #5248

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.

Sign up for GitHub

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

Merged
dmarcos merged 1 commit into from
Feb 26, 2023
Merged

Close a tiny gap in A-Frame loading order #5248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/core/a-assets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ class AAssets extends ANode {

connectedCallback () {
// Defer if DOM is not ready.
if (document.readyState === 'loading') {
Copy link
Member

@dmarcos dmarcos Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about if we check for document.readyState !== 'complete' instead? I think that would wait until deferred scripts have already run
Copy link
Member

@dmarcos dmarcos Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would need to call this.doConnectedCallback in the DOMContentLoaded event handler
Copy link
Member

@dmarcos dmarcos Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively we can listen to readystatechange and wait for document.readyState === 'complete' instead of listening to DOMContentLoaded
Copy link
Collaborator Author

@wmurphyrd wmurphyrd Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about if we check for document.readyState !== 'complete' instead? I think that would wait until deferred scripts have already run

If we checked against readyState complete but still listened for DOMContentLoaded, that would open another gap in the loading order:

  1. readyState: 'loading'
  2. readyState: 'interactive'
  3. Execute deferred scripts (including modules)
  4. DOMContentLoaded
  5. Execute DOMContentLoaded listeners
  6. readyState: 'complete'

During step 5, the readyState would still be interactive, so any entities attached from inside a DOMContentLoaded listener would never initialize as they would exit early from their connectedCallback and stay forever waiting for the DOMContentLoaded event that has already passed

would need to call this.doConnectedCallback in the DOMContentLoaded event handler

It does call through to doConnectedCallback as-is because the listener for the ANode static property is registered first and thus updates ANode.DOMContentHasLoaded before this comes back in via DOMContentLoaded listener. However, if you're open to updating all of these listeners to call doConnectedCallback I would love to make that change because it makes it much easier to subclass these when they call into doConnectedCallback via this rather than prototype.doConnectedCallback like how a-entity does currently

alternatively we can listen to readystatechange and wait for document.readyState === 'complete' instead of listening to DOMContentLoaded

Yes I proposed this as one of the two possible solutions when opening #5228. So that would look like:

if (readyState !== 'complete') {
  // vital that the callback here is connectedCallback and not doConnectedCallback
  // to make sure we don't initialize early on the loading->interactive transition
  document.addEventListener('readystatechange', this.connectedCallback.bind(this));
  return;
}

Note this would conflict with the previous suggesting about changing the callback to doConnectedCallback. If we still wanted to do that as well, we'd need to add an event handler function that checked the current readyState to make sure it was complete before calling doConnectedCallback
Copy link
Collaborator Author

@wmurphyrd wmurphyrd Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops on second thought the readystatechange solution proposed above would not work; it could stack listeners and initialize twice. Would have to go with the creating a readystatechange handler

onReadyStateChange () {
  if (document.readyState === 'complete') {
    this.doConnectedCallback()
  }
}

connectedCallback () {
  // Defer if DOM is not ready.
  if (document.readyState !== 'complete') {
    document.addEventListener('readystatechange', this.onReadyStateChange.bind(this));
    return;
  }
  this.doConnectedCallback()
}
Copy link
Member

@dmarcos dmarcos Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readystatechange

Thanks for all the info and patience. I think this solution looks simpler. Can we incorporate in the PR?
Copy link
Collaborator Author

@wmurphyrd wmurphyrd Feb 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good
Copy link
Collaborator Author

@wmurphyrd wmurphyrd Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmarcos I did the readystatechange version, but it occurred to me during the work that this could be a breaking change. Anyone using DOMContentLoaded listeners of their own with A-Frame probably expects the scene and entities to be initialized (although I guess that would depend on whether they registered their listerens before or after A-Frame), but this would change it so scene and entities are never initialized during DOMContentLoaded listener callbacks.

The unit tests may also be bearing this out, as the readystatechange version has one failure whereas this current version had none:

SUMMARY:
✔ 2302 tests completed
ℹ 16 tests skipped
✖ 1 test failed

FAILED TESTS:
    playSound
      ✖ plays sound if sound already playing when changing src
        Firefox 110.0 (Ubuntu 0.0.0)
      Timeout of 3000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

So just wanted to check in if you still want to go this direction with readystatechange. If so, I'll dig into the unit test failure
Copy link
Member

@dmarcos dmarcos Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In A-Frame the 'loaded' event should be used. DOMContentLoaded is not very useful since won't guarantee the scene, assets and entities are ready to go.
Copy link
Collaborator Author

@wmurphyrd wmurphyrd Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readystatechange version is up - that test failure I noted above - cannot get it to happen again. Tried several times locally and it passes in CI as well. Must have been a fluke.

This version still resolves my issues with deferred scripts & SVAWC
document.addEventListener('DOMContentLoaded', this.connectedCallback.bind(this));
if (document.readyState !== 'complete') {
document.addEventListener('readystatechange', this.onReadyStateChange.bind(this));
return;
}

Expand Down
10 changes: 8 additions & 2 deletions src/core/a-cubemap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,16 @@ class ACubeMap extends HTMLElement {
return self;
}

onReadyStateChange () {
if (document.readyState === 'complete') {
this.doConnectedCallback();
}
}

connectedCallback () {
// Defer if DOM is not ready.
if (document.readyState === 'loading') {
document.addEventListener('DOMContentLoaded', this.connectedCallback.bind(this));
if (document.readyState !== 'complete') {
document.addEventListener('readystatechange', this.onReadyStateChange.bind(this));
return;
}
ACubeMap.prototype.doConnectedCallback.call(this);
Expand Down
4 changes: 2 additions & 2 deletions src/core/a-entity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ class AEntity extends ANode {
*/
connectedCallback () {
// Defer if DOM is not ready.
if (document.readyState === 'loading') {
document.addEventListener('DOMContentLoaded', this.connectedCallback.bind(this));
if (document.readyState !== 'complete') {
document.addEventListener('readystatechange', this.onReadyStateChange.bind(this));
return;
}

Expand Down
4 changes: 2 additions & 2 deletions src/core/a-mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ class AMixin extends ANode {

connectedCallback () {
// Defer if DOM is not ready.
if (document.readyState === 'loading') {
document.addEventListener('DOMContentLoaded', this.connectedCallback.bind(this));
if (document.readyState !== 'complete') {
document.addEventListener('readystatechange', this.onReadyStateChange.bind(this));
return;
}

Expand Down
10 changes: 8 additions & 2 deletions src/core/a-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,16 @@ class ANode extends HTMLElement {
this.mixinEls = [];
}

onReadyStateChange () {
if (document.readyState === 'complete') {
this.doConnectedCallback();
}
}

connectedCallback () {
// Defer if DOM is not ready.
if (document.readyState === 'loading') {
document.addEventListener('DOMContentLoaded', this.connectedCallback.bind(this));
if (document.readyState !== 'complete') {
document.addEventListener('readystatechange', this.onReadyStateChange.bind(this));
return;
}
ANode.prototype.doConnectedCallback.call(this);
Expand Down
4 changes: 2 additions & 2 deletions src/core/scene/a-scene.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ class AScene extends AEntity {

connectedCallback () {
// Defer if DOM is not ready.
if (document.readyState === 'loading') {
document.addEventListener('DOMContentLoaded', this.connectedCallback.bind(this));
if (document.readyState !== 'complete') {
document.addEventListener('readystatechange', this.onReadyStateChange.bind(this));
return;
}

Expand Down