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

Use step-by-step credentials API (no functional change) #47898

Merged
merged 9 commits into from Dec 18, 2020

Conversation

delan
Copy link
Contributor

@delan delan commented Dec 1, 2020

Changes proposed

This patch makes the data layer for the update credentials pages use the new step-by-step credentials API, without actually changing any user-facing behaviour (which will happen in #47614).

Testing instructions

This patch depends on D53562-code and wpcom-xhr-request#31. If either or both have landed, skip the corresponding sections below. The calypso.live link will only work once both have landed.

D53562-code

Check out D53562:

$ arc patch D53562

Update your hosts(5) file to point public-api.wordpress.com to your sandbox (for more details, search the field guide for “Testing APIs in Your Sandbox”), then verify that it worked at ?amisandboxed (you may need to shift-reload).

Ensure that your hosts(5) file also has calypso.localhost and jetpack.cloud.localhost pointing to 127.0.0.1 or similar.

wpcom-xhr-request#31

On your Calypso dev machine, check out and build wpcom-xhr-request#31:

$ git fetch # or clone if necessary
$ git checkout add/streaming-response-support
$ npm install

Check out this patch:

$ git fetch # or clone if necessary
$ git checkout add/credentials-testing-progress-internals

Edit client/package.json to point .dependencies.wpcom-xhr-request to your local copy above:

{
	"dependencies": {
		"wpcom-xhr-request": "link:../../wpcom-xhr-request"
	}
}

Update your node_modules, then spin up either Calypso flavour locally:

$ yarn
$ yarn start # blue
$ yarn start-jetpack-cloud # green
Calypso tests

Open devtools on a patched version of Calypso, then run the following:

localStorage.debug = "calypso:data-layer:update-credentials"

Choose a Rewind-enabled site, then click Settings (green), or ManageSettingsJetpack (blue).

Verify that you can save working credentials successfully, with progress records sent to the console:

successscreenshot

Now try to save the same credentials except for an incorrect password or private key, and verify that you get the following error exactly once with no retry, and progress records sent to the console:

failure screenshot

@matticbot
Copy link
Contributor

client/state/data-layer/wpcom-http/actions.js Outdated Show resolved Hide resolved
expectStreamMode: true,

// TODO @azabani make this a requestDispatcher option to fully integrate with wpcom-http
onStreamRecord: ( record ) => debug( 'onStreamRecord: record=%o', record ),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is currently a WPCOM_HTTP_REQUEST option as a proof of concept, but yeah, i think the idea (in my next patch) would be to plumb this through meta like onSuccess + onError + onProgress?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(note that i’ve done exactly that in #47614)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if you want to be compatible with the data layer philosophy, then you don't use onStreamRecord as a callback. It should be a Redux action.

And you'll also need to enhance dispatchRequest to include the onStreamRecord option.

You want to look at how onProgress events flow through the code and implement something very similar for onStreamRecord.

The wpcom.req handler gets a onStreamRecord callback that dispatches an extended Redux action:

const onStreamRecord = ( record ) => {
  dispatch( extendAction( action.onStreamRecord, streamRecordMeta( record ) ) );
}

That dispatches the action.onStreamRecord action (it's an object with type etc.), enhanced with a meta.dataLayer.streamRecord property.

The dispatchRequest handler then intercepts this action, and needs to detect that there is the meta.dataLayer.streamRecord meta (in createRequestAction, with a check similar to getProgress). Then it will call its onStreamRecord action creator callback that receives action and record as arguments, and which returns an app-level action to dispatch and handle the record.

There are three functions/objects called onStreamRecord, and each of them is a different thing 🙁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the detailed walkthrough! i think #47614 should be aligned with every step of this, but let me know if i’ve missed anything.

@delan delan marked this pull request as ready for review December 1, 2020 13:35
@delan delan added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Rewind labels Dec 1, 2020
@delan delan requested a review from dmsnell December 3, 2020 05:15
@delan delan requested a review from jsnajdr December 4, 2020 10:11
@@ -64,6 +74,8 @@ export const http = (
onSuccess: onSuccess || action,
onFailure: onFailure || action,
onProgress: onProgress || action,
processResponseInStreamMode,
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need a boolean and a function? if the function is present, why would need to set the flag?

will processResponseInStreamMode !== ('function' === typeof onStreamRecord) ever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point! not in this patch, so i’ve dropped the flag from this layer in 1e53809.

but #47614 has to undo this, because we add on­Stream­Record to dispatch­Request. for requests that use dispatch­Request, we point on­Stream­Record to the default action no matter what, and i can’t think of a clean way to convey what this flag ought to be set to through create­Request­Action and the provided fetch function.

i think the only way to get rid of this flag is by also dropping it from wpcom-xhr-request, but i’m reluctant to do that for node.js safety reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i’ve since done exactly that in a684912 (thanks to @jsnajdr’s advice in Automattic/wpcom-xhr-request#31 (comment)).

@matticbot
Copy link
Contributor

matticbot commented Dec 8, 2020

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

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

name         parsed_size           gzip_size
entry-main        +352 B  (+0.0%)     +111 B  (+0.0%)
entry-login       +352 B  (+0.0%)     +125 B  (+0.1%)

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

Sections (~368 bytes added 📈 [gzipped])

name                    parsed_size           gzip_size
wpcom-xhr-request            +742 B  (+2.2%)     +281 B  (+3.3%)
settings-security            +311 B  (+0.1%)      +87 B  (+0.1%)
settings-jetpack             +311 B  (+0.1%)      +87 B  (+0.1%)
scan                         +311 B  (+0.1%)      +87 B  (+0.1%)
jetpack-cloud-settings       +311 B  (+0.2%)      +87 B  (+0.2%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~87 bytes added 📈 [gzipped])

name                                       parsed_size           gzip_size
async-load-signup-steps-rewind-form-creds       +311 B  (+0.6%)      +87 B  (+0.6%)
async-load-signup-steps-creds-permission        +311 B  (+0.9%)      +87 B  (+0.9%)
async-load-signup-steps-creds-confirm           +311 B  (+0.9%)      +87 B  (+0.9%)
async-load-signup-steps-clone-credentials       +311 B  (+0.5%)      +87 B  (+0.5%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

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.

@delan
Copy link
Contributor Author

delan commented Dec 17, 2020

@jsnajdr wpcom-http changes copied from #47614, ok to merge?

@delan delan changed the title Use step-by-step credentials API internally (no functional change) Use step-by-step credentials API (no functional change) Dec 17, 2020
Copy link
Member

@jsnajdr jsnajdr 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, thanks for all your work here! 👍

@delan delan merged commit 0f845ca into trunk Dec 18, 2020
@delan delan deleted the add/credentials-testing-progress-internals branch December 18, 2020 10:28
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 18, 2020
delan added a commit that referenced this pull request Dec 18, 2020
@delan delan restored the add/credentials-testing-progress-internals branch December 18, 2020 11:13
delan added a commit that referenced this pull request Dec 18, 2020
sarayourfriend pushed a commit that referenced this pull request Dec 23, 2020
* Use step-by-step credentials API internally (no functional change)

* Rename expectStreamMode to processResponseInStreamMode

* Make onStreamRecord imply pRISM in action builder

* Eliminate processResponseInStreamMode entirely (h/t @jsnajdr)

* Bump to wpcom-xhr-request@1.2.0

* Pull in client/state/data-layer/wpcom-http changes from #47614

* Update data-layer snapshot for installJetpackPlugin
sarayourfriend pushed a commit that referenced this pull request Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants