fix(engine-server): isConnected should not be true during construction
#1977
Conversation
| @@ -323,6 +323,11 @@ export function getAssociatedVM(obj: VMAssociable): VM { | |||
| return vm!; | |||
| } | |||
|
|
|||
| export function isConnected(obj: VMAssociable): boolean { | |||
| const vm = getAssociatedVM(obj); | |||
| return vm.state === VMState.connected; | |||
There was a problem hiding this comment.
since this is going away with node-reactions, this seems like the wrong abstraction.
|
IMO, on the server side, you can simply have a weakmap that allows you to track what elements are connected after core invokes the corresponding insertion mechanism (insertBefore/appendChild/etc). I don't think we will ever have removal on the server side, so, this should be easier to keep track of without having to expose anything from core. |
There was a problem hiding this comment.
Instead of creating a new API, I would rather look at the node.parent property. The DOM version of isConnected is more complex because the parent node might or not be connected to a Document. In the case of SSR, we can make a safe assumption that as soon as a node has a parent it is considered connected.
In order to make it work the renderComponent API has also to be updated to attached the rendered component to a fake root element before invoking connectRootElement.
|
@ekashida is this related to W-7532119? |
| <template shadowroot="open">Is connected? true</template> | ||
| <template shadowroot="open"> | ||
| <ul> | ||
| <li>connectedCallback</li> |
There was a problem hiding this comment.
IMO, the test should directly test that isConnected property
- in constructor is false
- in connectedCallback is true
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarks@lwc/engine-dom
|
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarks@lwc/engine-dom
|
⚠ Performance RegressionBest has detected that there is a Please click here to see more details. Click to view significantly changed benchmarks@lwc/engine-dom
|
ekashida
changed the title
true during construction
| import { HostElement, HostNodeType } from '../types'; | ||
|
|
||
| const FakeRootElement: HostElement = { | ||
| type: HostNodeType.Element, |
There was a problem hiding this comment.
should this be just a fragment type?
Details
When rendering on the server,
isConnectedshould reflect the current vm state.Does this PR introduce breaking changes?
No, it does not introduce breaking changes.The PR fulfills these requirements:
GUS work item
W-7391508