PERF: Eager load Theme associations in Stylesheet Manager. (PR #13397)

GitHub

The title of this pull request changed from “WIP” to "PERF: Eager load Theme associations in Stylesheet Manager.

Each request will work with an instance of Stylesheet::Manager which stores state of all the themes that has been loaded within a request. This allows us to reuse the state across method calls which wasn’t possible previously.

Drop support for extend here. It increases the matrix for caching leading to extra requests.

No need to call components_for is theme is not a parent theme.

This test has been 500ing leading to a false positive.

Drop support for querying for multiple themes. The previous behaviour was too confusing as it assumes the first element in the array is the parent theme id while the other elements in the array are the components theme ids of the parent.

This used to be Stylesheet::Manager but I need an instance of Stylesheet::Manager per request so moving this to another class as a builder. Copied and pasted instance methods from Stylesheet::Manager with some slight tweaks.

This is where all of the savings happen.

Test failure is a front end related.

    theme_ids = [theme_ids] unless Array === theme_ids

I think theme_ids and Array should switch places?

[5] pry(main)> [] === Array
=> false
[6] pry(main)> Array === a
=> true

I’ve left a few minor points/comments, but overall this looks great to me :ok_hand:

Shouldn’t this line be outside the unless customization_disabled? block? The customization_disabled? method returns true when themes are disable via safe mode, but server-side plugin outlets should still render when themes are disabled, no?

(customization_disabled? could probably use a better name that’s more accurate of what it does like themes_disabled? but it’s had that name forever, so it doesn’t need to be renamed in this PR)

I’m not 100% sure this is an issue, but I see we pass nil as theme_id when safe mode is enabled, and here we fallback to the default theme if theme_id is nil. I think it’s worth confirming that safe mode can still disable the default theme.

(I remember we had a bug like this a couple of years ago, but I’m not sure if tests were added when we fixed it)

Line 1 of this file needs to be updated to pass theme_id (singular) instead of theme_ids to discourse_stylesheet_link_tag.

Personal preference, but I prefer to use theme_ids.is_a? Array - this reads much nicer to me

Should we update the name of this meta tag? (and all the JS which references it?)

I think it might be good to add a validation here to make sure there isn’t anywhere still passing an array. I took this PR for a spin locally, and it looks like theme modifiers are broken because of this issue (ThemeModifierHelper and Theme.lookup_modifier handle arrays).

Easy way to try this out is to install Topic List Excerpts Theme Component - theme - Discourse Meta locally as a component.

Took this for a spin locally, seems to work well overall. A couple of issues I noticed:

  1. Theme modifiers are not working for components (see specific comment above)
  2. Previewing a component which has no theme CSS is completely broken. (e.g. install this component locally, click preview, everything breaks. If you add some simple CSS like body{color: red}, then things start working again)

Interesting, I was actually just copy and pasting the logic from application.html.erb and it seems like we have customization_disabled? and allow_plugins helper methods. I’ve split the rendering of the plugin html into another conditional.