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

Full Site Editing: Simplify how FSE blocks fetch posts #32727

Merged
merged 1 commit into from
May 3, 2019

Conversation

Copons
Copy link
Contributor

@Copons Copons commented May 1, 2019

Changes proposed in this Pull Request

In #32529, the FSE blocks fetching posts logic was based upon an apiFetch fired from a custom made fetchPost async function.
While not wrong per se, it forced an API call regardless of the presence of such post in state, and on top of that, didn't take advantage of the Redux/withSelect perks.

  • Remove the fetchPost utility, now unnecessary.
  • Use select( 'core' ).getEntityRecord( 'postType', postType, postId ) instead of the more convoluted custom mix of getEntityRecord and apiFetch proposed in fetchPost.
  • Make full use of HOC composition of withState and withSelect to request a new post (via getEntityRecord) only when a post has been selected and its ID and type have been stored as attribute, or into state (in the Content block, that doesn't persist it).

Testing instructions

Fixes Dennis' concerns in #32529 (review) (well, at least tries to 😄)

@Copons Copons added [Type] Enhancement [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Goal] Full Site Editing labels May 1, 2019
@Copons Copons requested review from dmsnell and a team May 1, 2019 15:11
@Copons Copons self-assigned this May 1, 2019
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

Awesome! (I didn't test this)

Copy link
Member

@vindl vindl left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@vindl vindl added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 3, 2019
@Copons Copons merged commit b59fc8a into master May 3, 2019
@Copons Copons deleted the fix/fse-fetch-post branch May 3, 2019 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants