Introducing 'cache' tag helper #1778
Conversation
|
cc @dougbu \ @NTaylorMullen |
| <h2>Cached content</h2> | ||
| @await Component.InvokeAsync("Splash") | ||
| @await Html.PartialAsync("_SplashPartial") | ||
| <div>CorrelationId in Splash: @ViewBag.CorrelationId</div> |
There was a problem hiding this comment.
@yishaigalatzer \ @davidfowl, I'm not super sure if we decided on what happens if we call Flush inside of the tag. Currently that's not handled, but I suppose that's something we want to support?
- Make it so that the content is buffered until the block is written and then invoke Flush. Keep track of this for the next time the tag is encountered and use that to flush for consecutive requests.
- Throw if you invoke Flush inside a
cachetag.
There was a problem hiding this comment.
Spoke to @DamianEdwards offline. We decided on going with the first approach. I'll follow up with a separate PR to reduce noise in this one.
|
⌚ |
| private const string CacheKeyTokenSeparator = "||"; | ||
| private static readonly char[] AttributeSeparator = new[] { ',' }; | ||
|
|
||
| [Activate] |
There was a problem hiding this comment.
Does this mean that we expect the memory cache to be registered as a service? I know there was a discussion happening here, not sure if this is the result or just a temp thing.
pranavkm
force-pushed
the
RazorCacheBase
branch
from
2373846 to
99ef875
Compare
| }); | ||
| } | ||
|
|
||
| // Clear the contents of the "cache" element since we don't want to render it. |
There was a problem hiding this comment.
The end goal is to not clear the contents, it's to clear the tag itself. Also, keep in mind by calling SupressOutput here you're essentially doing:
TagName = null;
PreContent = null;
Content = null;
PostContent = null;Meaning, any other TagHelper that targets the cache tag will have their PreContent/PostContent nuked, i'm not sure that's what we want to do. Also, I think it's worth mentioning that if the output.Content is already set at the beginning of the ProcessAsync (set by another TagHelper) method that we may want to consider no-oping; that's what we do for our other oob TagHelpers. /cc @DamianEdwards
There was a problem hiding this comment.
I'm not entirely sure I buy this - the cache tag helper is a custom element and additionally it's designed to not render any content by itself. There really isn't much you can do to augment it from a different tag helper.
There was a problem hiding this comment.
That's fair but it doesn't follow the same guidelines we use for the other built-in TagHelpers when working along side potentially other TagHelpers in the system.
There was a problem hiding this comment.
@NTaylorMullen none of the other MVC tag helpers have much in common w/ the <cache/> helper, so I'm not sure what guidance would carry over. this is the first time we're dealing with a helper that does not target an existing HTML element and that always throws away its own begin and end tags.
put another way, nuking TagName, PreContent, PostContent, and completely replacing Content is exactly what this helper should do.
|
⌚ |
pranavkm
force-pushed
the
RazorCacheBase
branch
from
99ef875 to
3b2df62
Compare
pranavkm
force-pushed
the
RazorCache
branch
2 times, most recently
from
5aacbba to
f629855
Compare
|
Updated per PR comments + added more functional tests. |
| /// Gets or sets the <see cref="ViewContext"/> for the current executing View. | ||
| /// </summary> | ||
| [Activate] | ||
| protected internal ViewContext ViewContext { get; set; } |
There was a problem hiding this comment.
FYI we'll likely make all of these properties public once [Activate] inherits from a Razor NotAnHtmlAttributeAttribute 😺
|
|
1 similar comment
|
|
|
Will merge this in once #1795 is done. |
|
Consider #1808 as part of this change |
| /// <summary> | ||
| /// Gets or sets a <see cref="string" /> to vary the cached result by. | ||
| /// </summary> | ||
| [HtmlAttributeName(VaryByAttributeName)] |
There was a problem hiding this comment.
I think these attributes are not necessary with @NTaylorMullen change
There was a problem hiding this comment.
Spoke to @NTaylorMullen and apparently the decision was to not make this change in this iteration.
Fixes #1552