-
Notifications
You must be signed in to change notification settings - Fork 8
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
Follow up performance improvements #325
Conversation
Remove protected array for current banners and move the cache contexts into th build method. Also amend the kernal test to account for #cache inside the render array.
@rupertj have managed to remove the need for storing current alert banners as a class property. Turns out it's not stored anyway between the build and getCacheContexts methods, so just applied it to the build render array. |
Looks good, although it'd be better to use CacheableMetadata, as that'd add all the relevant metadata from the node, not just contexts. (I'm thinking cache tags should come across too). |
@rupertj Yeah, I was looking at that, though we already have a getCacheTags and I think they should bubble. I can look at those in part 3 along with the manager service. |
I've just realised we're talking about a custom entity, and I don't think it implements the interface required to use CacheableMetadata right now, so doing it like this MR does is best for now anyway. |
Remove protected array for current banners and move the cache contexts
into th build method. Also amend the kernal test to account for #cache
inside the render array.