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.

Sign up for GitHub

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

Jump to bottom

Allow template caching to be customised #731

Merged
merged 10 commits into from
Jan 11, 2020
Merged

Allow template caching to be customised #731

merged 10 commits into from
Jan 11, 2020

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).

@phillipj phillipj mentioned this pull request Jan 5, 2020
Closed
@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
phillipj reviewed Jan 6, 2020
View reviewed changes
test/parse-test.js Outdated Show resolved Hide resolved
phillipj
phillipj reviewed Jan 6, 2020
View reviewed changes
mustache.mjs Outdated Show resolved Hide resolved
@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?

phillipj
phillipj approved these changes Jan 7, 2020
View reviewed changes
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 phillipj mentioned this pull request Jan 11, 2020
Merged
@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
Reviewers

@phillipj phillipj phillipj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow internal cache to be disabled
2 participants
@AndrewLeedham @phillipj