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

Further optimize for the expected case of "pure" style functions #10

Closed
dhleong opened this issue Mar 20, 2021 · 1 comment · Fixed by #17
Closed

Further optimize for the expected case of "pure" style functions #10

dhleong opened this issue Mar 20, 2021 · 1 comment · Fixed by #17

Comments

@dhleong
Copy link
Owner

dhleong commented Mar 20, 2021

In most cases, for any given input a style function should always return the same style. It turns out that we do not take advantage of that just now, but we could! Currently, whenever a style function is called, we:

  1. Call its "factory" function, which invokes the user-provided code and:
  2. Compiles the style spec into CSS, and:
  3. Generates a class name for the style+params
  4. If the style doesn't exist in the DOM, we create it; otherwise:
  5. If the CSS is changed, update the DOM

This is great for local dev, since the factory code might have changed since the last time we ran it, but in production for most cases, 1, 2, and 5 are completely unnecessary, and 3 probably ought to be memoized.

I propose for v1.2.0 we do some refactoring. The new flow would look like:

  1. Generate a class name for the style+params via a new, memoized function we generate. In the common case where the function takes no parameters, we might be able to inline this instead
  2. If the style doesn't exist in then DOM, we call the "factory" function to invoke the user-provided code and compile it into CSS, then create the DOM <style> element
  3. Otherwise, IF the (new) :always-compile-css? compiler flag is true (we will default to goog.DEBUG so this does not happen in prod by default) we call the "factory" function to generate CSS as above and, if changed, update the DOM

This should significantly reduce overhead for the hot path case of using a style function in a virtualized list—and, well, just in general—while not requiring any changes to user code for most cases, or having any impact on the dev cycle.

For weird cases where a style should always be rebuilt each time it's used we could add an escape hatch by annotating the style function with metadata:

(defattrs ^:always-compile-css not-referentially-transparent-attrs []
  {:color (js/Math.random) ; or whatever
   ...
   })

I honestly can't think of a good use case for this, however, so this escape hatch might be deferred until someone asks for it (unless it turns out to be very simple to add).

@dhleong
Copy link
Owner Author

dhleong commented Mar 20, 2021

One small wrinkle here is our (currently undocumented) support for manually specifying the class discriminator via :key metadata on the root object returned from the factory function. This is a neat idea in theory, because it makes class names a bit more readable in dev builds, but I don't think I've ever actually used it in practice because it's just extra work that really doesn't do much for you. I noticed that herb got rid of this feature as well, so there's prior art to that approach. They hash the generated style, however, instead of the params. I think we can continue hashing the params.

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 a pull request may close this issue.

1 participant