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

Add cache option #31

Closed
wants to merge 2 commits into from
Closed

Add cache option #31

wants to merge 2 commits into from

Conversation

wrapperup
Copy link
Contributor

This adds an option to disable the cache, useful if you want to iterate on templates in dev-time. The cache is still set (template internals still use them), but cache hits are ignored.

@oscarotero
Copy link
Member

Okay but I have two questions:

  • Why save the cache if it won't be used? A simpler option is to save the cache only if cache is enabled:
    if (this.options.cache) {
      this.cache.set(file, template);
    }
    Doing that, you don't need to check the cache in other places because it will be alwasy empty.
  • It's more performant to clear the cache after any file change. For example:
    function fileChanged() {
      env.cache.clear();
    }
    This allows to use the cache while the files doesn't change. Useful if you have imports inside a loop, like:
    {{ for number of 100 }}
      {{ import "./show_number.vto" { number } }}
    {{ /for }}
    
    Without cache, the "show_number.vto" file is loaded and compiled 100 times.

@wrapperup
Copy link
Contributor Author

wrapperup commented Feb 20, 2024

Clearing the cache is a nicer approach. I guess at some level this should be done in the integration, rather than here.

Perhaps instead there should be a split in the run API, one is user facing (we can clear the cache there) and the internal one plugins/etc can use.

If not, I think documenting this trick would be great.

@oscarotero
Copy link
Member

The run split would make the code more complicated, because it should pass the option to load, runString also use cache...

If clearing the cache works for you, I'd close this pull request. But feel free to open a new one if you want to contribute with the documentation!

Thanks!

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 this pull request may close these issues.

2 participants