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

Include/Render Statement Cache Not Refreshed When File Content Changed #602

Open
MrUpsideDown opened this issue Nov 10, 2023 · 3 comments
Open

Comments

@MrUpsideDown
Copy link

Firstly, thanks to all contributors for this excellent framework. We are using Fluid extensively in our open source project (Rock RMS), and it is a huge asset.

We are trying to resolve an issue with our Fluid implementation (v2.5), and would appreciate any thoughts on how to best address this problem.

The Include/Render Statements currently use an internal caching mechanism to avoid reloading content from the included file each time the statement is executed.
https://github.com/sebastienros/fluid/blob/main/Fluid/Ast/IncludeStatement.cs#L52
This makes good sense for performance, but in the current implementation the internal cache can only be refreshed if the name of the included file is changed. However, we would like to also refresh the cache if the file content is changed.

One way to achieve this could be to add a TemplateOptions.ContentProvider (superseding the existing TemplateOptions.FileProvider) that is tasked with supplying template content based on the provided key/filepath. The default implementation of the ContentProvider could mirror the existing implementation offered by the FileProvider, but could also (in our case) be swapped out for a different caching mechanism which can respond to other changes, such as file content modifications. Extending the provider in this way could also allow different content to be supplied for different rendering contexts.

Is this something that could/should be added to Fluid?

@MrUpsideDown
Copy link
Author

@sebastienros - I wonder if you have any thoughts on how we might move this forward? Is it something that would require a PR on our part, or is this kind of change best handled internally by you? Any comments appreciated!

@MrUpsideDown
Copy link
Author

@sebastienros - Just checking in to see if you have had a chance to consider this? We are very interested in finding a resolution for our project and would like to confirm if a PR would be the best way forward - thanks again for all your great work!

@sebastienros
Copy link
Owner

sebastienros commented Feb 2, 2024

Sorry for the delay. Here is my suggestion.

We still want the cache, and the parsed template cache is important. We could expose a TemplateCache class that has a ConcurrentDictionary<string, TemplateInfo> whith

public sealed class TemplateCache
{
  private ConcurrentDictionary<string, TemplateInfo> _templateInfos = [];

  public bool TryGetTemplate(string name, out var TemplateInfo);
  public TemplateInfo GetOrAddTemplate(string name, Func<string, IFluidTemplate> templateInfoBuilder);
  public bool TryRemoveTemplate(string name);
}

public sealed class TemplateInfo
{
  public string Name { get; set; }
  public IFluidTemplate Template { get; set; }
}

Then have this in the general options (each TemplateContext copies this instance when created, there are other properties doing it).

Finally IncludeStatement and RenderStatement will use if through TemplateContext to get/set the cached items. Check if other classes use some similar code maybe, I don't think so.

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

No branches or pull requests

2 participants