-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Defer initialization until end of aframe script or manual ready signal #5481
Conversation
src/core/a-node.js
Outdated
@@ -1,5 +1,6 @@ | |||
/* global customElements, CustomEvent, HTMLElement, MutationObserver */ | |||
var utils = require('../utils/'); | |||
var ready = require('./ready'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ready
is not a very descriptive word. Can we think of something better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps readyState
analogous to the document.readyState
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aframeReadyState?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return; | ||
} | ||
ANode.prototype.doConnectedCallback.call(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why this call has changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/core/a-node.js
Outdated
connectedCallback () { | ||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be isDOMReady
but not sure because isReady involves more than DOM now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isAnodeReadyToLoad
? too specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
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 |
How common would this pattern be? I don't have enough experience with modules
Seems this PR makes A-Frame loading logic more complicated and harder to understand. Just making sure there's a good payoff. |
@dmarcos This PR address more than just the dynamic imports. Without this PR none of the module based approaches work, nor classic scripts marked 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 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 |
a332055
to
9fb74d0
Compare
Can we add some docs? |
Co-authored-by: Chris Chua <[email protected]>
Added an entry to the FAQ, let me know if that suffices. At first I thought adding it to either |
Thanks! |
Description:
Updated and improved version of #5419. All
<a-node>
derived custom elements now wait for anaframeready
event. This event is emitted near the end of the aframe bundle OR once thedocument.readyState
becomescomplete
, whichever occurs last. In case the user needs to load more dependencies after this point, they can set thewindow.AFRAME_ASYNC
flag and manually provide the ready signal usingAFRAME.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 thedocument.readyState
has already reachedcompleted
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 bothfoo
andbar
, wherefoo
depends onbar
, in which case initializingfoo
beforebar
is registered causes issues)Changes proposed:
<a-node>
derived elements wait onaframeready
event instead ofreadystatechanged
aframeready
once the main bundle is done or thedocument.readyState
iscomplete
(whichever comes last)window.AFRAME_ASYNC
flag to allow users to manually signal A-Frame that everything is ready (throughAFRAME.ready()
)