-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat: store getComputedStyle result to avoid redoing that work #1048
Conversation
🦋 Changeset detectedLatest commit: 109ccbd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…table objects around
4a4bcfb
to
8ea82e3
Compare
// we don't cache the pseudoElement styles | ||
if (pseudoElement !== undefined) { | ||
return _getComputedStyle(el, pseudoElement); | ||
} |
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.
we could cache the pseudo element styles if the Map was of type MapLike<Element, {[key: pseudoElement]: CSSStyleDeclaration}>
, but that seemed kind of over the top
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.
it also didn't seem like it was possible they were called more than once per element
3ca0724
to
7c78f49
Compare
if (pseudoElement !== undefined) { | ||
return _getComputedStyle(el, pseudoElement); | ||
} | ||
const cachedStyles = computedStyles.get(el); |
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.
Worth calling out that this lookup is not the expected O(1) but O(N) which is especially important since this may negate any benefit we get for caching.
I feel like we should only cache when the built-in Map
is available. If you're testing in IE11, you don't need the caching anyway since you're in a browser already which has less issues with computing styles.
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.
that makes sense. i left a comment in the getComputedStyles
function about the polyfill being slower. let me know if you want me to document that somewhere else as well
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 saying we shouldn't use the polyfill at all and only cache when we know the lookup is O(1).
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.
sorry, i should have been clearer. i left a comment saying it was slower and that was why we were skipping using the caching if Map
is undefined
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 just removed the polyfill completely so there won't be a chance someone stumbles into the slow behavior
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 did end up having to include the global type for Map because otherwise TS complained about needing to change the target
d056107
to
f2b6eb5
Compare
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. I added a changelog entry for review.
sounds good. thanks! |
does some work to address #1011
![Screenshot 2024-06-07 at 4 04 54 PM](https://private-user-images.githubusercontent.com/8271228/337811101-a49a6d93-dfbd-45a7-8652-4f91d2c3e395.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU4NDUzNDQsIm5iZiI6MTcyNTg0NTA0NCwicGF0aCI6Ii84MjcxMjI4LzMzNzgxMTEwMS1hNDlhNmQ5My1kZmJkLTQ1YTctODY1Mi00ZjkxZDJjM2UzOTUucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDkwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA5MDlUMDEyNDA0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ZTg1Y2NkMjFmMjAzZDNhNjhkNzkzZjI3ZDIxZmNhZTU2NjEwOTFlYzZjMjI0YWNhOGE0NGVmNDE2ODc2OTQxNyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.JmD2pT0wVjhMQ-Abcoua_3TEZRJuXprhT4q4d__POxM)
![Screenshot 2024-06-07 at 4 02 59 PM](https://private-user-images.githubusercontent.com/8271228/337811116-24f23b18-380e-4a32-9043-11ca2f0ca8e2.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU4NDUzNDQsIm5iZiI6MTcyNTg0NTA0NCwicGF0aCI6Ii84MjcxMjI4LzMzNzgxMTExNi0yNGYyM2IxOC0zODBlLTRhMzItOTA0My0xMWNhMmYwY2E4ZTIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDkwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA5MDlUMDEyNDA0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NDU5M2Y4NDc0MjVkNTdmMTM3YmJjMWVkYjYwZWQ1YjI3MTgxZDFlMGQwMmNiMTQ5ZDdlODJlYTMyOThjZTRmOSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.sNDi-40Qax_auEzSFMQXz67KVqsiKR5nRs4jjH9ozv0)
all this does is store the results of callinggetComputedStyles
from theisHidden
function and reuse that later on when possible to avoid calculating it more than necessary. in the test it only saves a single extra call, but was 3x as fast in the test repo linked in that issueuse a caching version of
getComputedStyle
to avoid redoing that work since it's one of the more expensive functions calledupdated timings with the new implementation
before:
after: