Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~6 bytes added 📈 [gzipped]) DetailsCommon code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~194 bytes removed 📉 [gzipped]) DetailsSections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~109 bytes removed 📉 [gzipped]) DetailsReact 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. Generated by performance advisor bot at iscalypsofastyet.com. |
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
| { | ||
| "$schema": "http://json-schema.org/draft-04/schema#", | ||
| "properties": { | ||
| "data": { |
There was a problem hiding this comment.
IN my testing the data wrapper isn't included for the new wpcom/v3 endpoint. However, it is presumably included by the jetpack/v4 endpoint.
Therefore we actually need to fix the doJetpackFetchJITM calls as well otherwise we risk breaking JITMs in those contexts.
If anyone knows the best way to test that scenario it could save me a lot of time 🙏
There was a problem hiding this comment.
I've asked for some input in p1742832462839269-slack-C03N25JPCE4
There was a problem hiding this comment.
Left a response there. TL;DR: I believe that after Automattic/jetpack#39714 we don't really need special Jetpack site treatment.
There was a problem hiding this comment.
I tested three flavors of a JITM REST request and here are the results 🙂
/jetpack-blogs/:site/rest-api?path=/jetpack/v4/jitmreturns abodythat is of type{ data: [] }, i.e., the array with messages in on adatafield of the response JSON body.- the same
/jetpack/v4/jitmdone directly and locally on a Jetpack site returns abodythat is of type[], i.e. the messages array is the top-level object in the response JSON body. - the new
/wpcom/v3/sites/:site/jitmalso returns the array directly inbody, with nodatafield.
It turns out that adding data is a feature of the /jetpack-blogs/:site/rest-api endpoint. In the source (WPCOM_JSON_API_Jetpack_Blog_Endpoint::process_rest_api_request) there is a statement at the end that wraps the response:
return array(
'data' => $body,
);This PR removes usage of variant 1 (/jetpack-blogs) and replaces it with variant 3. That means that there are no longer responses that return a data field. Neither doFetchJITM (variant 3) nor doJetpackFetchJITM (variant 2) use it. That means that data can be removed from the schema. Is my reasoning correct?
The doJetpackFetchJITM flavor (local /jetpack/v4/jitm) request is used in Odyssey Stats when the Calypso-built widget is running inside a self-hosted Jetpack site's WP Admin. Maybe Automattic/jetpack#39714 disables that, but I don't immediately see how that disabling is done and have some doubts.
I also tested the locale query param and it doesn't seem to have any affect. But maybe it should! It makes sense to localize the messages. If the JITM tells me "Free domain with an annual plan", this message should be localized. I also tested the (native) _locale=de param but it doesn't work either.
There was a problem hiding this comment.
Currently looking for support in working out how to test in the "Jetpack" context as the endpoint is conditionally called.
To test this, just open a Stats page in Atomic site's WP Admin. Note that the stats UI code (webpack chunks for JS and CSS) is loaded from widget.wp.com/odyssey-stats/v1. It doesn't ship with the Jetpack plugin itself.
You'll see that the Odyssey app will be fetching JITMs from the local /jetpack/v4/jitm endpoint (wp:jetpack_page_stats:admin_notices) and renders a div with CSS class jetpack-jitm-message-container. Which will be empty unless there is actually a JITM to display.
This is the doJetpackFetchJITM code path at work 🙂
How does the the jetpack_display_jitms_on_screen filter in Automattic/jetpack#39714 fit into that? It disabled the "Jetpack native" UI to display JITMs on the Stats page, precisely because the Stats page now has its own UI to display JITMs, the one I just described above.
There was a problem hiding this comment.
I still have some doubts whether Post_Connection_JITM really does any essential work besides maybe caching.
True. Difficult to ascertain. We could have ignored it but who knows what relies on it.
It's worth trying to remove the additional complexity and just call JITM/Engine directly. But that's totally not a blocker today, it's more like a followup idea.
I think that's a totally valid observation. For Dotcom we will always be connected to JP so the Pre/Post Connection aspect is largely redundant.
The main point is that this body of work creates consistency is how we call and fetch JITMs across all sites and apps.
It also has the additional benefit of removing a redundant network request with Calypso proxying it's request to the endpoint on the JP site (which would in turn send the request to the Public API). Now Calypso just directly calls the public API.
There was a problem hiding this comment.
@jsnajdr Once this ships do you think we should remove the jetpack/v4/jitm endpoint? Or do we leave in place forever for backwards compatibility?
There was a problem hiding this comment.
Do you mean the one in the Jetpack plugin? Or the rest-api?path=/jetpack/v4/jitm one on WP.com? I'd rather keep both of them for now, at least for some time. Let's watch the stats how often they get called. My experience is that legacy resources very rarely drop to zero usage, there is always someone somewhere who loads them 🙂
There was a problem hiding this comment.
I thought they were the same endpoint?
rest-api?path=/jetpack/v4/jitmis calling thejetpack/v4/jitmon the remote JP sitejetpack/v4/jitmis in the Jetpack plugin and can be called directly or via the proxy
Happy to leave them around for now.
There was a problem hiding this comment.
I thought they were the same endpoint?
There's one caveat: rest-api?path=/jetpack/v4/jitm also works on Simple sites where there is no remote JP site to call. It's implemented in jetpack-direct-call, directly using the JITM Engine. It's another implementation that used directly the JITM primitives, it doesn't forward the REST request to another REST handler.
|
We need to update testing instructions for stats pages on:
This will ensure the JITMs work. Also we need to remember to updte instructions to say dimiss any Calypso notices else the JITMs aren't even requested 🤦 |
jsnajdr
left a comment
There was a problem hiding this comment.
Yes, this looks good. Thanks for the gruelling work getting this through the finish line 🙂
Thank you for helping out 🙇 |
|
Updated the docs to include a note about dismissing Notices in order to see JITMs. PCYsg-Pp7-p2 |
14400a0 to
9587076
Compare
|
Reaching out to original dev team on this for some support with Deployment. See p1744111326087799-slack-C034TMVE5CM |
Deployment Status
|
* Test using new endpoint * Update to restore comment * Update tests * Try running all calls to new endpoint * Allow calling local wpcom endpoint when running in Jetpack * Fix Post requests * Remove redundant code now shape is same * Resolve code review changes * Update tests to include isLocalApiCall * Move config check to runtime to enable mocking in tests * Provide test coverage for path change * revert whitespace change * Update with dedicated path tests
* Test using new endpoint * Update to restore comment * Update tests * Try running all calls to new endpoint * Allow calling local wpcom endpoint when running in Jetpack * Fix Post requests * Remove redundant code now shape is same * Resolve code review changes * Update tests to include isLocalApiCall * Move config check to runtime to enable mocking in tests * Provide test coverage for path change * revert whitespace change * Update with dedicated path tests
Related to #
Proposed Changes
Use the new
wpcom/v3/jitmendpoint on the Public API directly for non-Jetpack requests (see PCYsg-OT6-p2 for scenarios where Calypso code might run in a "Jetpack context").Note: JP contexts continue to use the Jetpack endpoint.
We can update that in a followup PR.Actually we can't because we're modifying theschema.jsonin this PR which would break those contexts. So we need to fix it!Why are these changes being made?
As part of p58i-kwy-p2 JITMs received a new endpoint in 177578-ghe-Automattic/wpcom.
This endpoint is available on the Public API and thus calling it directly avoids the additional network hops involved in calling the endpoint on the Jetpack site.
The current route for the request is: Calypso request -> Proxy request to JP site ->
jetpack/v4/jitm-> Public API JITM endpointThe proposed new route for the request is: Calypso request --> Public API JITM v3 endpoint.
The new v3 endpoint on the Public API routes all JITM requests via the Jetpack mechanics.
This change should improve the speed at which JITMs load in Calypso.
Testing Instructions
0-sandbox.php- 3774b-pb/#php/home/for a Simple site.Free domain with an annual planJITM in the sidebar.Networkin dev tools. Filter byjitm. Reload. Check that you can see requests going to Public APIwpcom/v3/jitm.Xicon on the JITM message in the UI. When clicked you should see aPOSTnetwork request to the JITM endpoint with the response beingtrue.Testing Odyssey stats:
0-sandbox.php- 3774c-pb/#phpcd ~/Sites/wp-calypso/apps/odyssey-statsScreenshots
Pre-merge Checklist