[feature request] change when upgrade/connected happens during parsing, so that it is the first event in the following microtask #787
Comments
|
As was explained in that issue, this is not the correct pattern to use for working with children, and we will not be changing things to accomodate broken patterns that try to inspect the children in the constructor or connectedCallback. |
|
Is there a pattern you'd recommend in particular for working with children? I'm thinking about having children emit an event to tell parents they are ready (in those children's If MutationObserver would fire
|
|
A MutationObserver watching for childList changes is the approach I would recommend. No built-in elements with child/parent relationships care about the connectedness of their children (as opposed to just looking at the connectedness of the parent), so I don't have a framework to advice you on the design you seem to be going for where that does matter for some reason. |
But it doesn't work during parsing. This codepen example shows that the So we need another way to work with initial children. |
|
I wish the |
|
Alright, here's an example using How do you feel about this CustomEvent pattern instead of the MO one? I think I'm liking this pattern because it is simpler, and could be applied to a custom element as a class-factory mixin which would also show what it could be like to ship new builtin features as mixins (referring to #758). |
|
I'm not sure what you mean by mutation observer doesn't get called for the initial children. It definitely gets called at least once in the following code as I tested in Safari 12.0.2 and Chrome 71.0.3578.98. It logs "child changed!" once then "child changed!" one second later as more child is inserted: <!DOCTYPE html>
<html>
<body>
<pre id="log"></pre>
<script>
customElements.define('test-parent', class extends HTMLElement {
constructor()
{
super();
this._observer = new MutationObserver(() => this.childChangedCallback());
this._observer.observe(this, {childList: true});
}
childChangedCallback()
{
log.textContent += 'child changed!\n';
}
});
setTimeout(() => {
document.querySelector('test-parent').appendChild(document.createElement('div'));
}, 1000);
</script>
<test-parent><div></div><div></div></test-parent>
</body>
</html> |
|
Look at your initial children using |
@domenic That doesn't work, children are not yet upgraded during the Otherwise I would be doing this and not opening these issues. |
|
@rniwa Yes, however you're missing a key point there: to avoid leaking memory and |
|
That seems like a very wrong way to use MutationObservers. @rniwa's approach is the correct one; I'm not sure where you got the idea that they "leak memory" or are "kept proper" by for some reason tying them to connectedCallback and disconnectedCallback. Mutations can happen any time; connected or not. |
Oh, so if we don't call
Because in general I try to allocate memory (create references to new objects) when needed, then unallocate memory (lose references to those objects and call relevant cleanup methods) so that I am sure things clean up. I have too much experience with devtools heap profiler to know that if I don't generally follow this pattern that I see things leak. |
That's why I simply want to handle initial children in a manual call (hence the deferrals, and |
|
So by "proper" I meant "good practice in defining how things are created AND reciprocally destroyed". |
|
For example, I would really hate to have a leak because of https://bugs.chromium.org/p/chromium/issues/detail?id=315190 and forgetting to clean things up. In general I'm trying to be cautious by reciprocally undoing whatever I do in all my components. I've seen to many leaks that were simply solved by just making sure to reciprocally clean things up. |
I'm still curious to know, what are both of your thoughts on that pattern? (considering it can be cleaned up in |
From my observations in Chrome, Similarly, all the Finally, connectedCallback or disconnectedCallback is not fired for any grandchildren of the root node if the root node is not in a document regardless if grandchildren are added or removed. On this note, I'd like to observe things in So the same sort of concept can apply to a |
|
Alright, I thought about it, a workaround for using the
A very big downside of this approach is that an observer is created even if the node is outside of a tree, and like I mentioned above in the previous comment, I'd rather make it behave the way This is bad because:
The
So taking @rniwa's example, run the following updated version showing the <!DOCTYPE html>
<html>
<body>
<pre id="log"></pre>
<script>
let instanceCount = 0
class TestElement extends HTMLElement {
constructor() {
super()
this.instanceNumber = ++instanceCount
this.changeCount = 0
this.createObserver()
}
connectedCallback() {
console.log('PARENT CONNECTED CALLBACK')
this.style = 'display: block'
this.createObserver()
}
disconnectedCallback() {
this.destroyObserver()
}
childConnectedCallback(child) {
console.log('CHILD CONNECTED CALLBACK')
this.changeCount++
this.appendChild(
document.createTextNode(
`child changed! Change count: ${
this.changeCount
}, Child tag: <${child.tagName.toLowerCase()}>, Child number: ${
child.instanceNumber
}`,
),
)
}
createObserver() {
if (this._observer) return
this._observer = new MutationObserver(changes => {
for (const change of changes) {
change.type === 'childList' &&
change.addedNodes &&
Array.from(change.addedNodes).forEach(child => {
if (!(child instanceof Element)) return
this.childConnectedCallback(child)
})
}
})
this._observer.observe(this, { childList: true })
}
destroyObserver() {
if (!this._observer) return
this._observer.disconnect()
this._observer = null
}
}
customElements.define('test-element', TestElement)
setInterval(() => {
document
.querySelector('test-element')
.appendChild(document.createElement('test-element'))
console.log(' --- Test tree outside of the document:')
// shows that the MutationObserver still operates on Nodes that are not in a document. :(
const one = document.createElement('test-element')
const two = document.createElement('test-element')
const three = document.createElement('test-element')
one.appendChild(two)
two.appendChild(three)
}, 1000)
</script>
<test-element>
Test1
<test-element>Test2</test-element>
<test-element>Test3</test-element>
</test-element>
</body>
</html>That's not a nice problem to have. The both APIs should be symmetrical! |
I see a way: check if |
|
Or just check isConnected. Note that it would return |
Ah, nice, thanks. The following is the CustomEvent-based version, which I just realized works only with Custom Element children that cooperate (emit the event), and not with builtins: CustomEvent example<!DOCTYPE html>
<html>
<body>
<script>
let instanceCount = 0
const childConnectedEvent = new CustomEvent('child-connected', { bubbles: false, composed: false })
const childDisconnectedEvent = new CustomEvent('child-disconnected', { bubbles: false, composed: false })
class TestElement extends HTMLElement {
constructor() {
super()
this.changeCount = 0
this.instanceNumber = ++instanceCount
this.__lastKnownParent = null
this.__childConnected = null
this.__childDisconnected = null
}
connectedCallback() {
this.__lastKnownParent = this.parentElement
this.__emitChildConnectedEvent()
this.__registerListeners()
this.style = 'display: block'
}
disconnectedCallback() {
this.__emitChildDisconnectedEvent()
this.__unregisterListeners()
this.__lastKnownParent = null
}
childConnectedCallback(child) {
console.log('CHILD CONNECTED CALLBACK')
this.changeCount++
this.appendChild(
document.createTextNode(
`child changed! Change count: ${ this.changeCount }, Child tag: <${child.tagName.toLowerCase()}>, Child number: ${ child.instanceNumber }`,
),
)
}
__emitChildConnectedEvent() {
childConnectedEvent.element = this // re-use a single event object (is this okay?)
this.__lastKnownParent.dispatchEvent(childConnectedEvent)
childConnectedEvent.element = null
}
__emitChildDisconnectedEvent() {
childDisconnectedEvent.element = this // re-use a single event object (is this okay?)
this.__lastKnownParent.dispatchEvent(childDisconnectedEvent)
childDisconnectedEvent.element = null
}
__registerListeners() {
this.__childConnected = event => this.childConnectedCallback(event.element)
this.addEventListener('child-connected', this.__childConnected)
this.__childDisconnected = event => this.childDisconnectedCallback(event.element)
this.addEventListener('child-disconnected', this.__childDisconnected)
}
__unregisterListeners() {
this.removeEventListener('child-connected', this.__childConnected)
this.removeEventListener('child-disconnected', this.__childDisconnected)
}
}
customElements.define('test-element', TestElement)
setInterval(() => {
document
.querySelector('test-element')
.appendChild(document.createElement('test-element'))
console.log(' --- Test tree outside of the document:')
// Nice! shows that nothing fires when the a tree is not in the document.
const one = document.createElement('test-element')
const two = document.createElement('test-element')
const three = document.createElement('test-element')
one.appendChild(two)
two.appendChild(three)
}, 1000)
</script>
<test-element>
Test1
<test-element>Test2</test-element>
<test-element>Test3</test-element>
</test-element>
</body>
</html> |
|
@rniwa It keeps getting more complicated: now if we create an instance with Now we're back at square one again, trying to do the same thing we couldn't do before without a macrotask! So if we want to fire Is there another way? Now I feel like I've ran in a circle. |
|
And then there's the issue of whether or not those children are defined yet or not, if they're elements with hyphens in their names. So if we are using React, which will create elements without them being |
|
I think the only option is to tell end users of the elements to ensure that the custom elements are always defined before they are ever used (f.e. define them in the Mainly I was trying to make everything "just work", even if for example the end user of the custom elements placed the script tag at the bottom of the page (defined the custom elements at the end of the body). To make this work is turning out to be such a PITA. |
|
This is bad, because what if an author doesn't control the load order of scripts? What if all they can do is import the element classes and define them in their module, which might just happen to be loaded at the end of the body. |
|
Another solution is to tell the user to bring back jQuery: $(document).ready(() => {
// now end user of the custom elements can begin manipulating them.
}) |
|
Alright, the following is the final solution with Don't mind the The implementation of import Class from 'lowclass'
import Mixin from 'lowclass/Mixin'
import { observeChildren } from '../core/Utility'
export default
Mixin(Base => Class('WithChildren').extends(Base, ({ Super, Private, Public }) => ({
// not sure if the custom element polyfills include this property, so we define it just in case.
isConnected: false,
constructor(...args) {
const self = Super(this).constructor(...args)
Private(self).__createObserver()
return self
},
connectedCallback() {
this.isConnected = true
Super(this).connectedCallback && Super(this).connectedCallback()
const priv = Private(this)
// NOTE! This specifically handles the case that if the node was previously
// disconnected from the document, then it won't have an __observer when
// reconnected. This is not the case when the element is first created,
// in which case the constructor already created the __observer.
//
// So in this case we have to manually trigger childConnectedCallbacks
if (!priv.__observer) {
const currentChildren = this.children
// NOTE! Luckily Promise.resolve() fires AFTER all children connectedCallbacks,
// which makes it similar to the MutationObserver events!
Promise.resolve().then(() => {
for (let l=currentChildren.length, i=0; i<l; i+=1) {
this.childConnectedCallback && this.childConnectedCallback(currentChildren[i])
}
})
}
Private(this).__createObserver()
},
disconnectedCallback() {
this.isConnected = false
Super(this).disconnectedCallback && Super(this).disconnectedCallback()
// Here we have to manually trigger childDisconnectedCallbacks
// because the observer will be disconnected.
const lastKnownChildren = this.children
// NOTE! Luckily Promise.resolve() fires AFTER all children disconnectedCallbacks,
// which makes it similar to the MutationObserver events!
Promise.resolve().then(() => {
for (let l=lastKnownChildren.length, i=0; i<l; i+=1) {
this.childDisconnectedCallback && this.childDisconnectedCallback(lastKnownChildren[i])
}
})
Private(this).__destroyObserver()
},
// private fields
private: {
__observer: null,
__createObserver() {
if (this.__observer) return
const self = Public(this)
this.__observer = observeChildren(
self,
child => {
if (!self.isConnected) return
self.childConnectedCallback && self.childConnectedCallback(child)
},
child => {
if (!self.isConnected) return
self.childDisconnectedCallback && self.childDisconnectedCallback(child)
},
true
)
},
__destroyObserver() {
if (!this.__observer) return
this.__observer.disconnect()
this.__observer = null
},
},
})))And here's how to use it: take your existing class, class MyElement extends HTMLElement {
connectedCallback() { /* ... */ }
disconnectedCallback() { /* ... */ }
}and mix the functionality in when you need it: import WithChildren from './WithChildren'
class MyElement extends WithChildren.mixin(HTMLElement) {
connectedCallback() { /* ... */ }
disconnectedCallback() { /* ... */ }
childConnectedCallback(child) { /* ... */ }
childDisconnectedCallback(child) { /* ... */ }
}Next I'd like to investigate if #789 can help make this cleaner.
|
|
My assumption is that I need to clean up the observer by calling If we don't have to call (For the sake of anyone not familiar with lowclass, I converted it to a regular import Mixin from '../core/Mixin'
import { observeChildren } from '../core/Utility'
export default
Mixin(Base => class WithChildren extends Base {
constructor(...args) {
super(...args)
// TODO, if the polyfills cover this remove it, isConnected is already
// part of the window.Node class (in the specs) and serves the same
// purpose. https://github.com/webcomponents/webcomponentsjs/issues/1065
this.isConnected = false
this.hasBeenDisconnected = false
this.__createObserver()
}
connectedCallback() {
this.isConnected = true
super.connectedCallback && super.connectedCallback()
if (this.hasBeenDisconnected) {
this.hasBeenDisconnected = false
// We have to manually trigger childConnectedCallbacks on connect.
const currentChildren = this.children
// NOTE! Luckily Promise.resolve() fires AFTER all children connectedCallbacks,
// which makes it similar to the MutationObserver events!
Promise.resolve().then(() => {
for (let l=currentChildren.length, i=0; i<l; i+=1) {
this.childConnectedCallback && this.childConnectedCallback(currentChildren[i])
}
})
}
}
disconnectedCallback() {
this.isConnected = false
this.hasBeenDisconnected = true
super.disconnectedCallback && super.disconnectedCallback()
// Here we have to manually trigger childDisconnectedCallbacks
// because the observer will be disconnected.
const lastKnownChildren = this.children
// NOTE! Luckily Promise.resolve() fires AFTER all children disconnectedCallbacks,
// which makes it similar to the MutationObserver events!
Promise.resolve().then(() => {
for (let l=lastKnownChildren.length, i=0; i<l; i+=1) {
this.childDisconnectedCallback && this.childDisconnectedCallback(lastKnownChildren[i])
}
})
}
__createObserver() {
observeChildren(
this,
child => {
if (!this.isConnected) return
this.childConnectedCallback && this.childConnectedCallback(child)
},
child => {
if (!this.isConnected) return
this.childDisconnectedCallback && this.childDisconnectedCallback(child)
},
true
)
}
})Again, note that Will this work? Can we expect things to be garbage collected when the node is not used anymore, without calling |
At the moment, it seems that during parsing, a custom elements code can not defer to a microtask in order to observe children (and expect the children to be upgraded).
I explained this in detail here: #550 (comment)
The main issue, put shortly, is that using
Promise.resolve().then()to defer to a microtask does not allow a custom element to observe their children after their children have already been upgrade. The only way is to defer to a macrotask withsetTimeout, which has the unwanted side effect of causing custom element code to execute after the code of any<script>tags that are located below the custom elements in parsing order.I'm not sure if this is the right place, but this is a feature request to change the behavior so that elements that are about to be upgraded will be upgraded in the first microtask following the connectedCallback of the current element. This way at least,
Promise.resolve().then()can be used by custom elements authors to work with already-upgraded children, without having to defer code beyond the execution of end-user code which may following the custom elements in parsing order.The text was updated successfully, but these errors were encountered: