wpcom-proxy-request: Return Promise if no callback specified

[ockham]

Changes proposed in this Pull Request

The promise wrapper introduced by @andrewserong et al. in #38823 seems generic enough for it to make sense to have it in wpcom-proxy-request. This PR moves it there. This follows precedent set by wpcom.js.

Note that this is a technically a breaking change, as noted in the package's History.md:

• [...] Return Promise (rather than XMLHttpRequest instance) if no callback argument is provided.
  • In practice, most people have probably been using the callback rather than the returned XMLHttpRequest instance, so this shouldn't be a breaking change for most.

Testing instructions

Verify that Gutenboarding still works (esp account and site creation).

[matticbot]

Test live: https://calypso.live/?branch=add/wpcom-proxy-request-promise-return

[sirreal]

More notes and thoughts than you wanted on this smallish PR 🙂

I'm particularly concerned about sideEffects - that likely breaks some functionality completely.

[ockham]

Thanks a lot for reviewing, Jon!

Addressed all feedback, should be ready for another look!

[p-jackson]

LGTM @ockham I just have a question about the calls to requestAllBlogsAccess() added in 989506c. Are they just there for testing purposes?

Doesn't seem related to this PR, but this call is needed for the stores to work properly perhaps we need a way of coordinating the calls so we don't create a network storm during app boot.

[ockham]

LGTM @ockham I just have a question about the calls to requestAllBlogsAccess() added in 989506c. Are they just there for testing purposes?

No, this was supposed to stay as-is. That's the call that was previously run at module level in packages/data-stores/src/utils/wpcom-wrapper.ts.

Doesn't seem related to this PR, but this call is needed for the stores to work properly perhaps we need a way of coordinating the calls so we don't create a network storm during app boot.

Is there a high risk for that? requestAllBlogsAccess() is now called from both the user and the site stores' createControls() function, which AFAICT should each be invoked only once -- in the register() function of each of these stores (site, user).

[sirreal]

Looks good, I don't see any regressions. Thanks!

[sirreal]

👍 thanks!

[sirreal]

It's worth noting that this is async, so we'll synchronously return the controls below, but may asynchronously error.

That seems alright to keep the happy path simple and failing fast.

[p-jackson]

That's the call that was previously run at module level

🤦‍♂ ah so it was

Is there a high risk for that? requestAllBlogsAccess() is now called from both the user and the site stores' createControls() function, which AFAICT should each be invoked only once -- in the register() function of each of these stores (site, user).

I shouldn't have said "network storm", that was the wrong term. Calling requestAllBlogsAccess() once per store is what I'm worried about. There's only two stores now, but what about when there are 5 stores, or 10 stores. An app that registers them all will fire 10 identical requests to the same endpoint on boot.

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~327 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
entry-main                  +235 B  (+0.0%)      +88 B  (+0.0%)
entry-login                 +235 B  (+0.0%)      +84 B  (+0.0%)
entry-jetpack-cloud         +235 B  (+0.0%)      +93 B  (+0.0%)
entry-domains-landing       +235 B  (+0.0%)      +72 B  (+0.0%)
entry-gutenboarding         +165 B  (+0.0%)      -10 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.