| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
| @@ -1,5 +1,6 @@ | |||
| /* global customElements, CustomEvent, HTMLElement, MutationObserver */ | |||
| var utils = require('../utils/'); | |||
| var ready = require('./ready'); | |||
There was a problem hiding this comment.
ready is not a very descriptive word. Can we think of something better?
Sorry, something went wrong.
There was a problem hiding this comment.
Perhaps readyState analogous to the document.readyState?
Sorry, something went wrong.
There was a problem hiding this comment.
aframeReadyState?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| document.addEventListener('aframeready', this.connectedCallback.bind(this)); | ||
| return; | ||
| } | ||
| ANode.prototype.doConnectedCallback.call(this); |
There was a problem hiding this comment.
Any reason why this call has changed?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| // Defer if DOM is not ready. | ||
| if (document.readyState !== 'complete') { | ||
| document.addEventListener('readystatechange', this.onReadyStateChange.bind(this)); | ||
| if (!ready.isReady) { |
There was a problem hiding this comment.
should be isDOMReady but not sure because isReady involves more than DOM now
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
isAnodeReadyToLoad? too specific?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@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). |
Sorry, something went wrong.
|
Can we add some docs? |
Sorry, something went wrong.
Co-authored-by: Chris Chua <chris.sirhc@gmail.com>
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. |
Sorry, something went wrong.
|
Thanks! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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
Deferred script (resolving #4038)
(Async) Inline Module
(Async) Module script file
Inline Module (w/ dynamic imports) (original problem #5419, see updated glitch)
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:
Whereas this would fail:
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: