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

expose calculateSelectorNode #23

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bartveneman
Copy link
Contributor

@bartveneman bartveneman commented Feb 18, 2024

closes #17

  • Exposes calculateSelectorNode next to the existing calculate function
  • Adds static Specificity.calculateSelectorNode(selector: string) + tests
  • Moved around some internals so calculateSelectorNode does not need to reference Specificity each time
  • Agree on naming
  • Check that /test/standalone is implemented correcty. I suspect the existing tests for calculate don't actually test it's implementation so I did mine a little different than the existing tests. Please double-check my work 😅. Also opened Question: are standalone tests correct? #24 for further discussion.

@bramus
Copy link
Owner

bramus commented Jun 20, 2024

Apologies for the delay but only now I took a good look into this PR.

One thing I’m wondering is why calculateSelectorNode returns a plain object while calculate does return (an array) with Specificity objects. I would have assumed for both to return Specificity instances

@bartveneman
Copy link
Contributor Author

bartveneman commented Jun 20, 2024

~~The existing function already returned a single object, so I simply renamed it and exported it.

It does make sense to me however to keep this method as small as possible, because it's easy to integrate and mock for testing.~~

edit: interpreted your question incorrectly

For my use case I generate sometimes over 10,000 Specificity instances, each with a small enough footprint. Separating instantiating the Specificity object felt light the right thing to do, to keep more of the 'business logic' in the more customer-facing calculate method. On the other hand, I don't have very strong feelings about this, so happy to revert that part.

@bartveneman
Copy link
Contributor Author

bartveneman commented Aug 10, 2024

My css-analyzer package has benchmarks enabled in CI and running it with the changes from this MR makes the analyzer 25-50% faster! projectwallace/css-analyzer#423 (comment)

(I don't necessarily trust the benchmarks blindly and ~50% is probably waaaay too much, but I'm very confident that it's faster because it's making a lot fewer function calls and memory allocations)

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

Successfully merging this pull request may close these issues.

Consider exposing calculateSpecificityOfSelectorObject
2 participants