Skip to content
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

Use node.append(child) instead of node.appendChild(child) #1037

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

skanne
Copy link
Contributor

@skanne skanne commented Feb 28, 2021

Hi Jorge,

similar to PR 1036 I suggest replacing node.appendChild(child) with node.append(child). Difference explained here: https://dev.to/ibn_abubakre/append-vs-appendchild-a4m.

Benefit: 5 characters/bytes of code less.

Regards,
Sven

@jorgebucaran
Copy link
Owner

Since append() accepts plain DOMStrings, I wonder if it's possible to take advantage of it around here:

hyperapp/index.js

Lines 131 to 136 in 9df1aca

} else if (
oldVNode != null &&
oldVNode.type === TEXT_NODE &&
newVNode.type === TEXT_NODE
) {
if (oldVNode.tag !== newVNode.tag) node.nodeValue = newVNode.tag

@jorgebucaran jorgebucaran added the enhancement New feature or request label Mar 1, 2021
@skanne
Copy link
Contributor Author

skanne commented Mar 1, 2021

Possibly. I'll spend some time on this and find out. I do see some potential around

hyperapp/index.js

Lines 105 to 110 in 9df1aca

var node =
vdom.type === TEXT_NODE
? document.createTextNode(vdom.tag)
: (isSvg = isSvg || vdom.tag === "svg")
? document.createElementNS(SVG_NS, vdom.tag, { is: props.is })
: document.createElement(vdom.tag, { is: props.is })
and

hyperapp/index.js

Lines 117 to 123 in 9df1aca

node.appendChild(
createNode(
(vdom.children[i] = maybeVNode(vdom.children[i])),
listener,
isSvg
)
)

But I need to wrap my head around it. High noon for me to try to understand all inner workings of Hyperapp's core. (Code comments would help, but are non-existent. Are you planning on documenting the core?)

@jorgebucaran
Copy link
Owner

High noon for me to try to understand all inner workings of Hyperapp's core.

What do you need help with?

@skanne
Copy link
Contributor Author

skanne commented Mar 1, 2021

Well, I need to walk through the code and see what happens. Never bothered to fully understand the concept of patching.

@l-cornelius-dol
Copy link

It would seem to me that here, as with #1036 the more generic function is likely to be less performant. At the least, it must inspect argument types and branch, whereas the dedicated function for Nodes expects a Node.

@skanne
Copy link
Contributor Author

skanne commented May 28, 2021

The performance difference really is neglible IMO. See https://www.measurethat.net/Benchmarks/Show/9176/0/js-append-vs-appendchild

@sergey-shpak
Copy link
Contributor

@skanne performance difference is about 12%, which is not critical for custom usage, but not for hyperapp(superfine)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants