Skip to content
This repository has been archived by the owner on Oct 4, 2019. It is now read-only.

Disable jetpack sync during REST requests #28

Merged
merged 2 commits into from
Jul 18, 2017

Conversation

allendav
Copy link
Contributor

Fixes #27

To test:

  • Use a tool like Postman to create a product using the REST API
  • Note the horribly long creation time (e.g. approx 5 seconds)
  • Install this branch
  • Create another product
  • Note the wonderfully short creation time (e.g. approx 2 seconds)
  • Profit!

Note: Bumps version to 0.7.0 also

cc @justinshreve or @DanReyLop for review
cc @westi

@allendav allendav self-assigned this Jul 18, 2017
Copy link

@DanReyLop DanReyLop left a comment

Choose a reason for hiding this comment

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

This is night and day. Thanks for a quick fix! :shipit:

@justinshreve
Copy link
Contributor

justinshreve commented Jul 18, 2017

I don't see any issues with the code. But I'm having some trouble seeing much improvements when testing :(. I'm not sure if I'm doing anything wrong, but I installed the new version, generated some keys, and did a simple POST to products from Postman.

On justin-devstore-at.blog, an Atomic site, I'm getting around 5 seconds with 0.6 of the plugin, and the same with 0.7 of the plugin. A few times I got less than 2, but I was able to get that in both cases occasionally. justin-devstore-at.blog doesn't have any additional plugins installed except for the ones WordPress.com & the Store setup add.

On wootest.mystagingwebsite.com I am getting sub 2 seconds for both versions.

It seems like 0.7 might be slightly faster, but I'm definitely not getting any huge improvements. It could just be me, or something else -- especially if you and others are definitely seeing a difference. Let me know if I can provide any more info.

@DanReyLop
Copy link

Wait, that's true, it's not working for me either. The last time I tried it I may have gotten lucky, I got a couple of less-than-1-second requests in a row and I assumed it was working.

I've added some logging and REST_REQUEST is not defined.

Copy link

@DanReyLop DanReyLop left a comment

Choose a reason for hiding this comment

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

.

@justinshreve
Copy link
Contributor

I added some debug right before the return:

echo 'defined? ' . defined( 'REST_REQUEST' );
echo 'REST_REQUEST ' . REST_REQUEST;
echo 'sender_should_load: ' . $sender_should_load;
defined? false 
REST_REQUEST REST_REQUEST
sender_should_load: 1

@justinshreve
Copy link
Contributor

Posting this from Slack:

from @DanReyLop

The jetpack_sync_sender_should_load filter is called in init, so at that point the REST_REQUEST constant isn't defined yet. This is gonna be more hacky than it should

I suggest maybe checking REQUEST_URI to see if it contains /wc/v3? That is the only idea I have. It's a bit hackier, but should still get the job done.

@allendav
Copy link
Contributor Author

Fixing this now. I only did 3 requests and they all came in at 2 seconds. I'll run the automated tests after fixing it to look at the request uri instead and make sure it is consistently better then

Copy link
Contributor

@justinshreve justinshreve 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. And is working consistently for me now. Definitely much much faster.

@allendav allendav merged commit 97f3d11 into master Jul 18, 2017
@allendav allendav deleted the fix/27-suspend-jp-sync-during-rest-response branch July 18, 2017 22:40
@allendav allendav restored the fix/27-suspend-jp-sync-during-rest-response branch July 19, 2017 15:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants