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

Support closures as values for ::set() #1

Merged
merged 9 commits into from
Dec 19, 2024
Merged

Conversation

borkweb
Copy link
Member

@borkweb borkweb commented Dec 19, 2024

This allows us to Memoize::set() with a Closure as a value, resolving it before setting in the cache.

Documentation Updates:

  • README.md: Updated to include a table of contents and examples using the new Memoizer class instead of the deprecated Memoize class. [1] [2] [3] [4]

Codebase Simplification:

New Features:

Testing Enhancements:

This allows us to `Memoize::set()` with a `Closure` as a value, resolving it when retrieving from memoization. If the key being retrieved is not itself a Closure, we will resolve the closures recursively from the key downwards.

Because `wp_cache_set()` should not store closures, when using the `WpCacheDriver`, any value that is set as a `Closure` will be stored in memory until the Closure is resolved.
Copy link

@JasonTheAdams JasonTheAdams left a comment

Choose a reason for hiding this comment

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

Oh interesting! You took it a slightly different way than I'd meant, which is fine! I originally meant that the function was called upon setting, so the driver doesn't matter. What you've done here is even more clever and it effectively makes a dynamic cached value, as the closure is called upon retrieval.

Honestly, I'm trying to decide if this is extremely clever or a caching anti-pattern. 🤔

This is based on some great recommendations from @defunctl and allows the library to be much more versatile - particularly in projects where Dependency Injection Containers are in use.
Implementing a better OOP structure
@borkweb
Copy link
Member Author

borkweb commented Dec 19, 2024

@JasonTheAdams - tbh, resolving on set would be waaaay simpler and I think I see where you are going with it being a potential anti-pattern. I could see how a result could become unpredictable later based on the moment of resolution.

@borkweb
Copy link
Member Author

borkweb commented Dec 19, 2024

I went ahead and went the resolve-on-set route for simplification's sake.

@JasonTheAdams JasonTheAdams self-requested a review December 19, 2024 19:44
Copy link

@JasonTheAdams JasonTheAdams left a comment

Choose a reason for hiding this comment

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

Looks good! 💪

@borkweb borkweb merged commit 0c2fe05 into main Dec 19, 2024
7 checks passed
@borkweb borkweb deleted the feature/closure-support branch December 19, 2024 19:49
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