cache visibility check results for reuse within a single operation #24
Conversation
Thank you for making this change and proving that it is a fix. However, before we proceed I need to do one more little thing - I want to track the existing "complexity" to understand the difference.
|
|
Complexity check has been merged - #25 - and everything is VERY BAD Please merge with a master and lets see much much crap will be gone. |
src/utils/is.ts
Outdated
| return false; | ||
| } | ||
| const cached = visibilityCache.get(node) | ||
| if(cached) { |
There was a problem hiding this comment.
that is working only for "true" cases, while false is legit as well
| if(cached) { | |
| if (cached!==undefined) { |
src/utils/is.ts
Outdated
| return cached; | ||
| } | ||
| // @ts-ignore | ||
| const result = node === document || |
There was a problem hiding this comment.
can we extract the actual check to the separate function?
Technically you can just rename "the old one" and then build a cached version next to it.
Codecov Report
@@ Coverage Diff @@
## master #24 +/- ##
==========================================
+ Coverage 82.69% 82.86% +0.17%
==========================================
Files 20 20
Lines 312 321 +9
Branches 68 69 +1
==========================================
+ Hits 258 266 +8
- Misses 49 50 +1
Partials 5 5
Continue to review full report at Codecov.
|
|
Applied the suggestions, thanks! Regarding an extra test, I think the footprint one you added will cover it well. It shows how much the number of calls reduced |
|
Yes! And shows how much. |
|
ok, so the fix got merged, but to benefit from it we need to have a release Is there anything blocking this? |
|
I was just not satisfied with the result numbers and added a few more tests. The result numbers are WAY better, and (again) that's all thanks to you 🎉 |
| @@ -7,7 +7,7 @@ const isElementHidden = (computedStyle: CSSStyleDeclaration): boolean => { | |||
| ); | |||
| }; | |||
|
|
|||
| export const isVisible = (node: HTMLElement | undefined): boolean => | |||
| const isVisible = (node: HTMLElement | undefined): boolean => | |||
| !node || | |||
| // @ts-ignore | |||
| node === document || | |||
There was a problem hiding this comment.
oh no. Nested call is to the same "uncached" isVisible
|
Published as focus-lock@0.9.1 |
|
Very nice! Thanks for doing proper quality assurance of my code :) |
`react-focus-lock@2.5.1` includes a dependency update of `focus-lock` from `0.8.1` -> `0.9.1`. The change in `focus-lock` includes a fix for performance in JSDOM: theKashey/focus-lock#24 JSDOM is used when testing react components in jest and other unit testing frameworks. In particular, when used with `@testing-library/react` for simulating real user input. Locally tested on an Apple M1 Air using a moderately complex `<Modal>` component (which contained inputs, `react-hook-form` usage, etc). Before this change: 20,149ms After this change: 2,347ms Approx. 10x performance increase.
`react-focus-lock@2.5.1` includes a dependency update of `focus-lock` from `0.8.1` -> `0.9.1`. The change in `focus-lock` includes a fix for performance in JSDOM: theKashey/focus-lock#24
My naive attempt to fix: theKashey/react-focus-lock#151
I confirmed this in my real application case, it prevents Jest test from getting timed out.