-
Notifications
You must be signed in to change notification settings - Fork 5
Add LazyHTML.Tree.postreduce/3 and LazyHTML.Tree.prereduce/3 #15
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
Conversation
lib/lazy_html/tree.ex
Outdated
This function traverses the tree without modifying it, check `postwalk/2` and | ||
`postwalk/3` if you need to modify the tree. | ||
""" | ||
@spec traverse( |
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.
In Elixir, Macro.traverse is both prewalk and postwalk combined. So I think we should pick a different name here... maybe postwalk_reduce
?
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.
postvisit
, postscan
, since they better indicate read-only?
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.
postscan
is also fine by me. Or postreduce
.
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.
I like postreduce
!
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.
8e33368
renamed to postreduce
lib/lazy_html/tree.ex
Outdated
(html_node(), acc -> acc) | ||
) :: acc | ||
when acc: term() | ||
def traverse(tree, acc, fun), do: do_traverse(tree, acc, fun) |
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.
No need for the do_traverse
btw. You can implement them all directly as clauses of traverse
.
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.
Btw, I applied this patch to LiveView, and it seems they need a |
Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>
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.
Thank you!
Just to be clear, this won't work for LV. We need a prereduce there. |
@ypconstante let's add both then :D |
Done, also updated the benchmark to show the difference between adding children to the list of items to reduce vs just calling the reduce multiple times. Previous benchmark showed pre order traversal as being slower, but it was just the algorithm difference, performance is basically the same for both approaches. |
Phoenix LiveView is calling
LazyHTML.Tree.postwalk
in many situations in which it just needs to traverse the tree, without modifying its nodes.This PR adds a
LazyHTML.Tree.postreduce/3
function to optimize this kind of scenario in which the tree won't be modified.Benchmark done using Floki benchmark HTML files.