FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Defer initialization until end of aframe script or manual ready signal by mrxz · Pull Request #5481 · aframevr/aframe · GitHub

Defer initialization until end of aframe script or manual ready signal - #5481

Merged
dmarcos merged 1 commit into
aframevr:masterfrom
mrxz:async-loading
Mar 15, 2024
Merged

Defer initialization until end of aframe script or manual ready signal#5481
dmarcos merged 1 commit into
aframevr:masterfrom
mrxz:async-loading

Conversation

mrxz commented Feb 28, 2024
edited
Loading

Copy link
Copy Markdown
Contributor

Description:
Updated and improved version of #5419. All <a-node> derived custom elements now wait for an aframeready event. This event is emitted near the end of the aframe bundle OR once the document.readyState becomes complete, whichever occurs last. In case the user needs to load more dependencies after this point, they can set the window.AFRAME_ASYNC flag and manually provide the ready signal using AFRAME.ready()

The following configurations are now all supported.

Classic script

<head>
    <script src="aframe-master.js"></script>
    <script src="components-and-systems.js"></script>
</head>

Deferred script (resolving #4038)

<head>
    <script src="aframe-master.js" defer></script>
    <script src="components-and-systems.js" defer></script>
</head>

(Async) Inline Module

<head>
    <script type="importmap">(...)</script>
    <script type="module" async> <!-- works with and without async -->
        import 'aframe';
        import 'components-and-systems';
    </script>
</head>

(Async) Module script file

<head>
    <script type="importmap">(...)</script>
    <script type="module" src="index.mjs" async></script> <!-- works with and without async -->
</head>

Inline Module (w/ dynamic imports) (original problem #5419, see updated glitch)

<head>
    <script type="importmap">(...)</script>
    <script type="module">
        window.AFRAME_ASYNC = true;
        await import("aframe")
        await import("components-and-systems");
        window.AFRAME.ready();
    </script>
</head>

You only really need to use AFRAME_ASYNC + AFRAME.ready() if the aframe main bundle is loaded after the document.readyState has already reached completed and you asynchronously register new components or systems. This means asynchronously compared to the aframe main bundle, so the following works:

import 'aframe';
AFRAME.registerComponent('new-component', { ... });

Whereas this would fail:

import 'aframe';
setTimeout(() => {
    AFRAME.registerComponent('new-component', { ... });
});

The way it fails is interesting in that it doesn't give errors and the components/system will work fine for the most part. But since they are registered after the scene has initialized, any attributes that weren't recognized as components or systems won't trigger component initialization. Effectively the runtime loading of components and systems could be improved. However, users should probably opt for AFRAME_ASYNC unless they know what they are doing, as cross component dependencies could still cause issues (say a library registers both foo and bar, where foo depends on bar, in which case initializing foo before bar is registered causes issues)

Changes proposed:

  • Let all <a-node> derived elements wait on aframeready event instead of readystatechanged
  • Emit aframeready once the main bundle is done or the document.readyState is complete (whichever comes last)
  • Introduce window.AFRAME_ASYNC flag to allow users to manually signal A-Frame that everything is ready (through AFRAME.ready())

Comment thread src/core/a-node.js Outdated
@@ -1,5 +1,6 @@
/* global customElements, CustomEvent, HTMLElement, MutationObserver */
var utils = require('../utils/');
var ready = require('./ready');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ready is not a very descriptive word. Can we think of something better?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Perhaps readyState analogous to the document.readyState?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

aframeReadyState?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As the local var name that works, though as file name I'd say readyState.js suffices as the aframe prefix won't make things clearer IMHO.

Comment thread src/index.js Outdated
Comment thread src/core/a-node.js
document.addEventListener('aframeready', this.connectedCallback.bind(this));
return;
}
ANode.prototype.doConnectedCallback.call(this);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Any reason why this call has changed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, the inheritance was a bit wonky. The doConnectedCallback function was essentially setup as a "virtual function" with subclasses overriding it and calling the base implementation (super.doConnectedCallback()), yet connectedCallback explicitly called the ANode implementation, forcing all subclasses to also override connectedCallback and replicating the same logic. From an OOP standpoint this didn't really make sense. Having one shared connectedCallback implementation is the cleanest solution IMO.

Comment thread src/core/a-node.js Outdated
// Defer if DOM is not ready.
if (document.readyState !== 'complete') {
document.addEventListener('readystatechange', this.onReadyStateChange.bind(this));
if (!ready.isReady) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

should be isDOMReady but not sure because isReady involves more than DOM now

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, it now encompasses both DOM (= all elements are parsed) and A-Frame (everything registered, incl. third-party components/systems/etc...) being ready. Not sure what an apt name would be.

Perhaps the name could be changed to not reflect what needs to be ready, but that this action can take place. So something like canInitializeElements?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

isAnodeReadyToLoad? too specific?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Feels a tad specific, but it also reads as if "Anode" is ready, instead of everything being ready "for Anode" to load. And we'd probably want to avoid using "load" here as the doConnectedCallback implementations do more than just invoke load.

dmarcos commented Mar 13, 2024

Copy link
Copy Markdown
Member

We should add to docs and maybe README (if we can keep it simple) the different ways to load A-Frame listed in this PR description

dmarcos commented Mar 13, 2024
edited
Loading

Copy link
Copy Markdown
Member

How common would this pattern be? I don't have enough experience with modules

<head>
    <script type="importmap">(...)</script>
    <script type="module">
        window.AFRAME_ASYNC = true;
        await import("aframe")
        await import("components-and-systems");
        window.AFRAME.ready();
    </script>
</head>

Seems this PR makes A-Frame loading logic more complicated and harder to understand. Just making sure there's a good payoff.

mrxz commented Mar 14, 2024

Copy link
Copy Markdown
Contributor Author

@dmarcos This PR address more than just the dynamic imports. Without this PR none of the module based approaches work, nor classic scripts marked defer. Literally the only way to load A-Frame would be a blocking synchronous classic script. The dynamic import case in question only requires the emitReady method being exposed and the window.AFRAME_ASYNC condition in index.js.

While dynamic imports aren't the most common in my experience, #5419 raises a valid usage. On top of that more complex module setups (e.g. using top-level awaits) might also need window.AFRAME_ASYNC + emitReady despite not using dynamic imports.

Having this escape hatch is useful in those situations. Another option would be to treat module based imports differently by always requiring the user to manually signal everything is ready. That would eliminate the need for the magic AFRAME_ASYNC, but it would break any app out there that already got A-Frame working this way with their own workarounds (which, given the various issues on this subject over time, are probably more than a few).

mrxz force-pushed the async-loading branch 2 times, most recently from a332055 to 9fb74d0 Compare March 14, 2024 10:01
Comment thread src/index.js Outdated

dmarcos commented Mar 14, 2024

Copy link
Copy Markdown
Member

Can we add some docs?

mrxz commented Mar 15, 2024

Copy link
Copy Markdown
Contributor Author

Can we add some docs?

Added an entry to the FAQ, let me know if that suffices. At first I thought adding it to either best-practices.md, or installation.md, but it just detracts from the focus of those articles. In general I would expect people either setting it up the "old traditional way", and for those in an ES module environment simply loading "aframe". In most cases that should now just work. When they do run into issue due to asynchronous loading of additional components/systems after loading A-Frame, there's now a point in the docs we can point them to.

dmarcos commented Mar 15, 2024

Copy link
Copy Markdown
Member

Thanks!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL