Skip to content

Commit

Permalink
Resolve issue with conditionally rendering elements
Browse files Browse the repository at this point in the history
  • Loading branch information
@asynts
asynts committed Nov 17, 2022
1 parent 1f41ad6 commit 9f85082
Show file tree
Hide file tree
Showing 4 changed files with 131 additions and 38 deletions.
112 changes: 112 additions & 0 deletions notes/0013_2022-11-17_conditional-rendering-is-broken.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,120 @@
This suggests that we are matching with some node that isn't even being rendered.
This looks completely broken.
- Somehow, we have a `NO_MATCH` when rendering the root component?
This is because it's a `HtmlNode` and the other is a `ComponentNode`.

This was resolved by adding a `root-container` element.

- I tried to debug how we are matching.
This is what I expected:

```none
// initial render
NO_MATCH_NEW #1
NO_MATCH_NEW #2
NO_MATCH_NEW #3
NO_MATCH_NEW #4
NO_MATCH_NEW #5
// first click (hide)
DIRECT_MATCH #1
SKIP_MATCH #4
DIRECT_MATCH #4 (implied by SKIP_MATCH)
DIRECT_MATCH #5
// second click (show)
DIRECT_MATCH #1
NO_MATCH #2
NO_MATCH #3
DIRECT_MATCH #4
DIRECT_MATCH #5
```

This is what I got:

```none
react-mini.js:159 handle_NO_MATCH_NEW
index.js:314 HtmlNode {nextSibling: null, elementType: 'DIV', children: Array(2), properties: {…}, renderedElement: null}
react-mini.js:421 HtmlNode.renderChilden null
react-mini.js:429 -> render HtmlNode {nextSibling: HtmlNode, elementType: 'P', children: Array(1), properties: {…}, renderedElement: null} null
react-mini.js:159 handle_NO_MATCH_NEW
react-mini.js:421 HtmlNode.renderChilden null
react-mini.js:429 -> render TextNode {nextSibling: null, text: 'This is not always visible', properties: {…}, renderedElement: undefined} null
react-mini.js:159 handle_NO_MATCH_NEW
react-mini.js:429 -> render HtmlNode {nextSibling: null, elementType: 'BUTTON', children: Array(1), properties: {…}, renderedElement: null} null
react-mini.js:159 handle_NO_MATCH_NEW
react-mini.js:421 HtmlNode.renderChilden null
react-mini.js:429 -> render TextNode {nextSibling: null, text: 'Toggle Visiblity', properties: {…}, renderedElement: undefined} null
react-mini.js:159 handle_NO_MATCH_NEW
react-mini.js:107 handle_DIRECT_MATCH
index.js:314 HtmlNode {nextSibling: null, elementType: 'DIV', children: Array(1), properties: {…}, renderedElement: null}
react-mini.js:421 HtmlNode.renderChilden HtmlNode {nextSibling: HtmlNode, elementType: 'P', children: Array(1), properties: {…}, renderedElement: p}
react-mini.js:429 -> render HtmlNode {nextSibling: null, elementType: 'BUTTON', children: Array(1), properties: {…}, renderedElement: null} HtmlNode {nextSibling: HtmlNode, elementType: 'P', children: Array(1), properties: {…}, renderedElement: p}
react-mini.js:126 handle_SKIP_MATCH
react-mini.js:107 handle_DIRECT_MATCH
react-mini.js:421 HtmlNode.renderChilden TextNode {nextSibling: null, text: 'Toggle Visiblity', properties: {…}, renderedElement: text}
react-mini.js:429 -> render TextNode {nextSibling: null, text: 'Toggle Visiblity', properties: {…}, renderedElement: undefined} TextNode {nextSibling: null, text: 'Toggle Visiblity', properties: {…}, renderedElement: text}
react-mini.js:107 handle_DIRECT_MATCH
react-mini.js:107 handle_DIRECT_MATCH
index.js:314 HtmlNode {nextSibling: null, elementType: 'DIV', children: Array(2), properties: {…}, renderedElement: null}
react-mini.js:421 HtmlNode.renderChilden HtmlNode {nextSibling: null, elementType: 'BUTTON', children: Array(1), properties: {…}, renderedElement: button}
react-mini.js:429 -> render HtmlNode {nextSibling: HtmlNode, elementType: 'P', children: Array(1), properties: {…}, renderedElement: null} HtmlNode {nextSibling: null, elementType: 'BUTTON', children: Array(1), properties: {…}, renderedElement: button}
react-mini.js:140 handle_NO_MATCH
react-mini.js:421 HtmlNode.renderChilden TextNode {nextSibling: null, text: 'Toggle Visiblity', properties: {…}, renderedElement: text}
react-mini.js:429 -> render TextNode {nextSibling: null, text: 'This is not always visible', properties: {…}, renderedElement: undefined} TextNode {nextSibling: null, text: 'Toggle Visiblity', properties: {…}, renderedElement: text}
react-mini.js:107 handle_DIRECT_MATCH
react-mini.js:429 -> render HtmlNode {nextSibling: null, elementType: 'BUTTON', children: Array(1), properties: {…}, renderedElement: null} HtmlNode {nextSibling: null, elementType: 'BUTTON', children: Array(1), properties: {…}, renderedElement: button}
react-mini.js:107 handle_DIRECT_MATCH
react-mini.js:421 HtmlNode.renderChilden TextNode {nextSibling: null, text: 'Toggle Visiblity', properties: {…}, renderedElement: text}
react-mini.js:429 -> render TextNode {nextSibling: null, text: 'Toggle Visiblity', properties: {…}, renderedElement: undefined} TextNode {nextSibling: null, text: 'Toggle Visiblity', properties: {…}, renderedElement: text}
react-mini.js:107 handle_DIRECT_MATCH
```

From this it looks like we advance the `oldNode` in the first `NO_MATCH`.

- I found an error that may be involved in this:

```js
if (renderedElement === undefined) {
this.renderedElement = renderedElement;
} else {
this.renderedElement = null;
}
```

should have been:

```js
if (renderedElement !== undefined) {
this.renderedElement = renderedElement;
} else {
this.renderedElement = null;
}
```

That issue seems to be unrelated though, this is simply why there is `renderedElement===undefined` in the logs.

- For `OLD_MATCH` we can't compute the `oldFirstChild`, it doesn't exist!

### Ideas

- Verify that the logic in `ConditionComponent` makes sense.

- Walk through what should happen if all works as intended.

### Theories

- I suspect, that `renderChildren` isn't always called when it should be.
- I suspect, that the `render` in `renderChildren` matches with the completely wrong thing.
### Result
- Add `root-container` div instead of replacing it.
That resolves the issue where the root component is not equal to the placeholder element.
- The problem was that in `OLD_MATCH` I was trying to compute the `oldFirstChild` which doesn't make any sense.
Instead I am now passing `{ oldFirstChild: null }` to `renderChildren`.
2 changes: 1 addition & 1 deletion src/index.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@
<link rel="stylesheet" href="index.css" />
</head>
<body>
<div id="root"></div>
<div id="root-container"></div>
</body>
</html>
18 changes: 10 additions & 8 deletions src/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -258,18 +258,18 @@ function ConditionComponent({ useState }) {
let [visible, setVisible] = useState("visible", true);

/*
<div #1 key="root class="component">
{visible && <p #2 key="1">
<text #3 key="1">This is not always visible!</text>
</p>}
<div #1 key="root" class="component">
{visible &&
<p #2 key="1">
<text #3 key="1">This is not always visible!</text>
</p>}
<button #4 key="2" $click={() => setVisible(!visible)}>
<text #5 key="1">Toggle Visibility</text>
</button>
</div>
*/

let node_5 = new TextNode({
text: "Toggle Visibility",
text: "Toggle Visiblity",
properties: {
key: "1",
},
Expand All @@ -287,7 +287,7 @@ function ConditionComponent({ useState }) {
let node_3 = new TextNode({
text: "This is not always visible",
properties: {
key: "4", // FIXME: Setting this to "1" changes the issue.
key: "1",
},
});
let node_2 = new HtmlNode({
Expand All @@ -303,13 +303,15 @@ function ConditionComponent({ useState }) {
elementType: "div",
properties: {
key: "root",
class: "component",
},
children: [
...(visible ? [node_2] : []),
node_4,
],
});

console.log(node_1);
return node_1;
}

Expand Down Expand Up @@ -380,6 +382,6 @@ let component = new ComponentNode({

new Instance()
.mount(
document.getElementById("root"),
document.getElementById("root-container"),
component,
);
37 changes: 8 additions & 29 deletions src/react-mini.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,14 +142,9 @@ export class Node {

oldNode.renderedElement.parentElement.insertBefore(this.createElement(), oldNode.renderedElement);

let oldFirstChild;
if (oldNode !== null && oldNode.getChildren().length >= 1) {
oldFirstChild = oldNode.getChildren()[0];
} else {
oldFirstChild = null;
}

this.renderChildren({ oldFirstChild });
this.renderChildren({
oldFirstChild: null,
});

return oldNode;
};
Expand Down Expand Up @@ -298,7 +293,7 @@ export class TextNode extends Node {
ASSERT("key" in properties);
this.properties = properties;

if (renderedElement === undefined) {
if (renderedElement !== undefined) {
this.renderedElement = renderedElement;
} else {
this.renderedElement = null;
Expand Down Expand Up @@ -328,7 +323,7 @@ export class TextNode extends Node {
return [];
}

renderChildren() {
renderChildren({ oldFirstChild }) {
// Has no children.
}
}
Expand Down Expand Up @@ -460,26 +455,10 @@ export class HtmlNode extends Node {
}

export class Instance {
mount(markerTargetElement, newNode) {
// We want the root element to be in a well defined state.
let oldElement = document.createElement("div");
oldElement.setAttribute("key", "root");
markerTargetElement.replaceWith(oldElement);

// Create the corresponding node.
let oldNode = new HtmlNode({
elementType: "div",
properties: {
key: "root",
},
children: [],
renderedElement: oldElement,
});

// Render.
mount(rootContainerElement, newNode) {
newNode.render({
oldNode,
parentElement: null,
oldNode: null,
parentElement: rootContainerElement,
});
}
}

0 comments on commit 9f85082

Please sign in to comment.