-
Notifications
You must be signed in to change notification settings - Fork 834
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
Performance: only add one resize listener with makeVisFlexible #90
Conversation
} | ||
} | ||
|
||
function remove(list, item) { |
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.
Is there a helper/pattern already in the codebase for removing from an array?
58a5404
to
7922055
Compare
} | ||
} | ||
|
||
function remove(list, item) { |
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.
Could you rename remove
to something more specific? e.g. removeListener
?
7922055
to
dd476db
Compare
// if we have no Flexible components, remove the listener | ||
if (resizeSubscribers.length == 0) { | ||
window.clearTimeout(timeoutId); | ||
window.removeEventListener('resize', emit); |
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 should be debounceEmitResize
@btford , I highly suggest to add descriptions for the functions that you have. JSDoc is not enforced in this project, but can be a good option. |
@bulyonov Done. |
dd476db
to
fbdf582
Compare
Performance: only add one resize listener with makeVisFlexible
Performance: only add one resize listener with makeVisFlexible
In a somewhat un-scientific test on my local machine, I saw a 50% reduction in latency when you have 4-5 flexible components.
I think we should discuss this a bit before merging, but here's the basic idea:
Rather than have each instance add an event listener to the window, we consolidate the subscripting into a global queue. This also means that we only have one debounce-er for the event as well.
The same idea should be applicable to anywhere that a component uses
componentDidMount
andcomponentWillUnmount
to add event listeners.The current implementation introduces a couple extra helpers. I think we can simplify this since
react-vis
only needs this pattern in one place.Another note: this change removes the possibility for setting the debounce for resize on a per-component basis. I personally think that this is silly– reacting to the resize multiple times due to a difference in debounce rate would likely degrade performance.
The current PR makes the debounce fixed at 100ms. I'm not sure what the best way to expose a global debounce setting would be. Are there any other such global settings in react-vis?