-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Code golf diffChildren size down by 33 B ⛳️ #1580
Conversation
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.
This looks wonderfull, also clearly documented as to how and why you replaced it. Thanks a lot for this great PR
let oldChildren = oldParentVNode!=null && oldParentVNode!=EMPTY_OBJ && oldParentVNode._children || EMPTY_ARR; | ||
// This is a compression of oldParentVNode!=null && oldParentVNode != EMPTY_OBJ && oldParentVNode._children || EMPTY_ARR | ||
// as EMPTY_OBJ._children should be `undefined`. | ||
let oldChildren = oldParentVNode!=null && oldParentVNode._children || EMPTY_ARR; |
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.
This seems safe to me. My guess is the original checks were a combination of before + after oldParentVNode
became reliably a (vnode | null)
.
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.
So good! 🌈
This pull request reduces the gzipped code size by 33 bytes by progressively refactoring
diffChildren
. The commits are roughly in an increasing order of weirdness, so just some of them can be cherry-picked if needed.break
statements (-7 B)p
andindex
, with hopefully sufficient comments explaining the logic (-13 B)EMPTY_OBJ._children
isundefined
(-4 B)These changes conflict a bit with @andrewiggins's #1578, sorry about that.