-
Notifications
You must be signed in to change notification settings - Fork 649
feat: Use fastdom to batch writes and avoid sync layout #385
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
Open
STRML
wants to merge
6
commits into
reactjs:master
Choose a base branch
from
BitMEX:feat/fastdom
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
5d0a9ed
feat: Use fastdom to batch writes and avoid sync layout
STRML 2541850
refactor: remove visibility
STRML 41c08e2
fix(fastdom): fix animations by triggering reflow
STRML e71fed5
fix(CSSTransition): don't add undefined class
STRML ec881ae
fix(cssTransition): Don't rely on 'active' in className
STRML c0ae113
perf(classHelpers): reflow all pending nodes completely before class …
STRML File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
import fastdom from 'fastdom'; | ||
import addOneClass from 'dom-helpers/class/addClass'; | ||
import removeOneClass from 'dom-helpers/class/removeClass'; | ||
|
||
export function addClass(node, classes, reflow) { | ||
mutateClass(node, classes, reflow, addOneClass); | ||
} | ||
export function removeClass(node, classes, reflow) { | ||
mutateClass(node, classes, reflow, removeOneClass); | ||
} | ||
|
||
function mutateClass(node, classes, reflow, fn) { | ||
if (!node) return; | ||
if (classes && typeof classes === 'string') { | ||
const run = () => { | ||
// A reflow is necessary to get the browser to respect the transition. However, it doesn't | ||
// need to be done on every single class change, only when the 'active' class is added. | ||
// Batching reflows allows us to avoid read-write-read context switches, by reading | ||
// (node.scrollTop) completely before we write. | ||
if (reflow) emptyReflow(); | ||
classes.split(' ').forEach(c => fn(node, c)); | ||
} | ||
// If possible, on browsers, batch these mutations as to avoid synchronous layouts. | ||
// But watch out - if we are batching them, and the page is inactive, we can end up | ||
// hitching up the page for a very long time as these callbacks queue up on | ||
// requestAnimationFrame. So only queue them up if the page is visible. | ||
if (process.browser && document.hidden === false) { | ||
// Is this an entering animation where we'll need to force a reflow? | ||
if (reflow) reflowNodes.push(node); | ||
// Schedule the modification for next tick. | ||
fastdom.mutate(run); | ||
} else { | ||
run(); | ||
} | ||
} | ||
} | ||
|
||
const reflowNodes = []; | ||
// Empty the `reflowNodes` array completely and reflow all of them at once. | ||
function emptyReflow() { | ||
let node; | ||
while (node = reflowNodes.pop()) forceReflow(node); | ||
} | ||
|
||
// This is for to force a repaint, | ||
// which is necessary in order to transition styles when adding a class name. | ||
function forceReflow(node) { | ||
/* eslint-disable no-unused-expressions */ | ||
node && node.scrollTop; | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why is the reflow being removed?
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.
The point is to avoid synchronous layout, which this forces. In my testing it was no longer necessary when using
fastdom
as the class manipulation happens on another tick.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'm a bit concerned about this, bc my previous testing trying to use rAF directly instead of the sync reflow led to animations not running. I wish I had a specific test case to try tho, I don't honestly remember what the cases were. Maybe during interrupts or fast toggling?
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 haven't been able to replicate any problems in my testing - ideas?
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.
theoretically, what is ensuring that
active
classes are being added on a different tick than the initial classes? From what I can tell here, every call tofastdom.mutate
adds to the same queue, if the initial and active classes are added within the the rAF time frame (very likely?) they will flush on the same tick.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.
To follow up on this, we've been using this in prod for 3mo+ without any issue. The forced reflow is unnecessary as fastdom will delay the change until the next animation frame.
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.
The
-active
classes work as expected, I tested this by addingopacity: 0
to-enter
andopacity: 1
to-enter-active
. The element faded in, which wouldn't happen if they were added in the same tick.