Skip to content
This repository has been archived by the owner on Sep 5, 2018. It is now read-only.

Cache retrieved model from getter in embedded #158

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

westonruter
Copy link
Member

Builds on #157. The one new commit is 07fea24.
Needs unit tests.

getModel.fetch( {
success: function( model ) {
var updatedEmbeddeds = _.clone( parentModel.get( '_embedded' ) || {} );
if ( ! updatedEmbeddeds[ embedSourcePoint ] ) {

Choose a reason for hiding this comment

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

@westonruter can you explain why you are caching the parent embed here? does this prevent duplicate lookup calls? can you explain how to reproduce and i can work on a unit test to demonstrate what this fixes? thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly. If we cache the fetched embedded model inside of the parent's _embedded then subsequent calls will avoid doing external calls, and will instead return what was previously fetched.

The scenario where I encountered this problem was when I introduced a related post REST field with a getRelatedPost model getter, and I wanted to getFeaturedMedia of that related post: https://github.com/xwp/wp-customize-featured-content-demo/blob/6243ce237ab2974d5d17b1d4a8895169bff3e67b/js/frontend.js#L8-L51

Choose a reason for hiding this comment

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

Great, thanks, that should help figure out a unit test. I started working on a core merge changeset, I need to fix a few docs fixes that went into core I don't want to overwrite. https://github.com/adamsilverstein/develop.wordpress/tree/pr157. I verified the existing client unit tests all pass and we'll get a run on that branch in travis now as well - the travis:js run includes the wp-api client tests - https://travis-ci.org/adamsilverstein/develop.wordpress

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants