Skip to content
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

[meta] Custom metrics #46

Open
LeaVerou opened this issue Jul 26, 2020 · 8 comments
Open

[meta] Custom metrics #46

LeaVerou opened this issue Jul 26, 2020 · 8 comments

Comments

@LeaVerou
Copy link
Owner

LeaVerou commented Jul 26, 2020

After wading through all proposed stats, it looks like the only ones that may require custom metrics are:

  • Going deeper on custom-property usage #1 Some of the metrics here require (or would benefit from) access to computed styles. I think it may be best to store a tree of variable computed values and which selectors they come from (which should be pretty small). This way we can expand what we study if needed. I can write the JS for that.
  • Analyze Sass stylesheets #39 This will definitely require custom metrics, and its feasibility is debatable.
  • CSS-in-JS #44 just because I don't know enough about CSS-in-JS to have the faintest idea how to tackle this metric

Greg's script in #37 would also fall under that category, since it reads document.styleSheets, but it may be a better idea to write a script that uses the Rework AST to compute these metrics.

CC @rviscomi @dooman87

@rviscomi
Copy link
Collaborator

I think #44 is the only one that's feasible with custom metrics

@LeaVerou
Copy link
Owner Author

@rviscomi Why not #1? I've been working on JS for it and I think we can store a very compact JSON object with valuable data for a variety of CSS variable metrics.

@rviscomi
Copy link
Collaborator

rviscomi commented Jul 27, 2020

IIUC, it's an O(n) metric that grows with the number of elements/styles on the page. Pages could have hundreds of thousands of elements, which could have many styles, so I'm worried about the storage costs incurred by the metric. If there are any tests or sample data you could share, that'd be helpful to better understand the impact. By the sound of #1 (comment) the solution also seems overly complex.

@LeaVerou
Copy link
Owner Author

We can avoid the O(n)-ness by only exploring elements that match the selectors found in the stylesheet if total count of DOM nodes exceeds a certain threshold. It would mean we miss some data, but not a lot. I'm working on this today, so I'll have a prototype for you by tonight.

@dooman87
Copy link
Collaborator

@LeaVerou Regarding #44, there is a list I found with more or less popular CSS-in-JS libraries - https://github.com/A-gambit/CSS-IN-JS-Benchmarks/blob/master/RESULT.md. Each of the libraries would need different detection algorithm. I don't think we can put them all in. Do you reckon a subset would be sufficient?

@LeaVerou
Copy link
Owner Author

@dooman87 Sure, a subset of the most popular ones should suffice. I trust you to pick the right subset :)

@LeaVerou
Copy link
Owner Author

IIUC, it's an O(n) metric that grows with the number of elements/styles on the page. Pages could have hundreds of thousands of elements, which could have many styles, so I'm worried about the storage costs incurred by the metric. If there are any tests or sample data you could share, that'd be helpful to better understand the impact. By the sound of #1 (comment) the solution also seems overly complex.

Not sure if you saw this, I committed a prototype last night. Not sure if I should just directly add it to css.js and iterate there, or what's the recommended way to debug these things. But it's not O(n), unless the website uses custom properties in overly liberal selectors (e.g. in *), which should be very rare (how rare? We could compute this as one of the stats!). It traverses document.styleSheets and elements with custom properties in the inline styles, and then goes from there, instead of walking the entire DOM tree.

@rviscomi
Copy link
Collaborator

Thanks @LeaVerou! The prototype looks good and is much more compact than I had imagined. Let's try to get it added ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants