Skip to content

Conversation

@gravityrail
Copy link
Contributor

@gravityrail gravityrail commented May 30, 2019

In order to provide Jetpack API endpoints to other plugins, we move these files into a package.

The near-term goal is to have VaultPress able to connect and sync.

We aim to achieve this as follows:

Step 1: Move API files into package

In this step, we attempt to modify as little code as possible, but simply to make the API available in the package.

  • Move legacy API classes mostly unmodified into the /legacy directory
  • Replace the code which requires these files with $loader->loadClass( 'WPCOM_JSON_API_Some_Endpoint' ); (for each class that was in the file)

Step 2: Virtualise API

Currently the REST API has to load the code for every API class, even though on any given request only one of those classes will actually be used to respond. This fills up the opcache with unneeded classes. Many APIs are actually never (or rarely) used, plus requiring them all foregoes the benefits of autoloading itself.

In order to virtualise the API, we need to change the pattern by which endpoints are registered. Currently the pattern is this:

new WPCOM_JSON_API_Some_Endpoint( array(
	'some' => 'params',
	'method' => 'POST',
	'path'        => '/sites/%s/something',
) );

This requires the endpoint class to be defined. Instead, we should have a way of registering an endpoint along with the class responsible for servicing these requests, but only load the class itself if the request is known to be for that class.

This may require something like:

new WPCOM_JSON_API_Virtual_Endpoint( 'WPCOM_JSON_API_Some_Endpoint', array(
	'some' => 'params',
	'method' => 'POST',
	'path'        => '/sites/%s/something',
) );

The "Virtual" endpoint would instantiate the class on the fly if the incoming request is a POST to '/sites/%s/something'

Step 3: Separate endpoint definitions from implementations

We may not want to ship all our API implementations with every plugin. Yet, we should still be able to differentiate between an API which doesn't exist at all, vs one which requires a package that the plugin didn't include.

This will allow us to both ship less code to plugins, and have API clients be smarter about what they tell the user. For example, if VaultPress includes just a handful of APIs, and a site doesn't have Jetpack, Calypso should be able to function properly and simply hide, warn, etc. for endpoints that are not present on the site. Worst case scenario, it could tell the user that in order to display some screen, they need to install the Jetpack plugin.

@gravityrail gravityrail requested a review from a team May 30, 2019 18:36
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello gravityrail! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D28924-code before merging this PR. Thank you!

@jetpackbot
Copy link
Collaborator

jetpackbot commented May 30, 2019

Warnings
⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 026451e

@roccotripaldi
Copy link
Contributor

We've merged this into the feature branch already:
#12527

@dereksmart dereksmart reopened this May 31, 2019
@dereksmart dereksmart changed the base branch from master to feature/jetpack-packages May 31, 2019 20:29
@lezama lezama requested a review from a team May 31, 2019 21:26
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

It would be interesting to deal with cases where endpoints rely on functionality that's not there. For example, the sync status endpoint would require the sync package; how would we behave if the sync package isn't here? Should we define an endpoint that returns an error, or not define it at all? Should we require sync package as a dependency of this one? 🤔

"url": "./packages/*"
"url": "./packages/*",
"options": {
"symlink": true
Copy link
Member

Choose a reason for hiding this comment

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

Hm, could we remove this? Ideally, we should be symlinking by default but mirroring for production.

@lezama
Copy link
Contributor

lezama commented Jun 4, 2019

Should we define an endpoint that returns an error, or not define it at all? Should we require sync package as a dependency of this one? 🤔

What about each package registering their own endpoints? when the package is not present we could just 404

@dereksmart dereksmart changed the base branch from feature/jetpack-packages to feature/jetpack-packages-2 June 7, 2019 21:41
@dereksmart dereksmart closed this Jun 10, 2019
@dereksmart dereksmart reopened this Jun 10, 2019
@dereksmart dereksmart changed the base branch from feature/jetpack-packages-2 to master June 17, 2019 14:17
@gravityrail
Copy link
Contributor Author

This approach is stale. Closing.

@gravityrail gravityrail deleted the try/api-package branch June 27, 2019 19:12
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.

10 participants