Skip to content

Allow template caching to be customised#731

Merged
phillipj merged 10 commits intojanl:masterfrom
AndrewLeedham:master
Jan 11, 2020
Merged

Allow template caching to be customised#731
phillipj merged 10 commits intojanl:masterfrom
AndrewLeedham:master

Conversation

@AndrewLeedham
Copy link
Contributor

fixes: #730

Not 100% sold on this approach but this allows for parse to be more cleanly overridden without having to clear the cache for every call. Open to suggestions.

@phillipj
Copy link
Collaborator

Cool @AndrewLeedham! Thanks for submitting an approach to disabling the cache when needed 👍

For the sake of trying to keep the related discussion in one thread, I posted a longer reply in #730 (comment).

@AndrewLeedham
Copy link
Contributor Author

@phillipj I have implemented your recommendations from #730, took a bit to get the tests to pass, but all is working now.

Should this stay undocumented?

@phillipj
Copy link
Collaborator

phillipj commented Jan 6, 2020

Great job ensuring these changes works on legacy aswell 👍

Should this stay undocumented?

Good question! As setting a custom template cache is only needed for a selected few, I'm leaning towards not documenting disabling / custom cache in the README until we get lots of requests about how it works...

Although some explanation would probably be valuable, both for us selfs and future collabs, maybe it would be fitting to explain the concept as a block comment above the set templateCache that gets introduced here?

Copy link
Collaborator

@phillipj phillipj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good to me.

Let's keep this PR up for a couple of days before merging, just in case someone else feels like chiming in.

@phillipj phillipj changed the title Allow caching to be disabled Allow template caching to be customised Jan 8, 2020
@phillipj phillipj merged commit e77fc7c into janl:master Jan 11, 2020
@phillipj
Copy link
Collaborator

These changes has been published to npm as v4.0.0.

Thanks a lot for the great initiative and this PR 👍

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.

Allow internal cache to be disabled

2 participants