Skip to content

Conversation

@pyronaur
Copy link
Contributor

@pyronaur pyronaur commented Feb 6, 2023

This is a follow-up implementation of:

Proposed changes:

This implements both wp-js-data-sync composer package and the svelte-data-sync-client JavaScript package.

I've written documentation for both packages, have a look at the README.md files in this PR for details.

Brief overview:
We needed a streamlined way to interact with WordPress REST API while also having data pre-loaded using wp_localize_script and ended up with a lot of spaghetti code to manage data flow between JavaScript and WordPress.

This streamlines the process so that data is defined once and then used on the front-end using the utilities this PR provides.

This allows to:

  • Have a clear understanding of what options are available
  • Ensure that any necessary transformation/validation/sanitization actions are performed at predictable times
  • Provide a structured way to pass the option data to the frontend on-page load
  • Automatically generate REST API CRUD endpoints that can be used by the Svelte front-end
  • Automatically set up a nonce for every option that is registered
  • Have typed Svelte options stores that automatically sync with WordPress backend

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

N/A

Does this pull request change what data or activity we track or use?

No

Testing instructions:

  1. Apply the patch
  2. Read the documentation
  3. Implement test features

commit b37c81b31999d6a2e23115460d1fe1f14c3f9b40
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 21:55:53 2023 +0200

    Add changelog entries

commit b69b6a7fbee66ddd6d3e8d5f22a2d5f2f3ad7e67
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 21:48:54 2023 +0200

    🌈 Fix eslint for web pack

commit 58d62d7b42bf962066b0b818529a9c9b0b1c6fff
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 21:44:57 2023 +0200

    Run phpcs

commit 9a3db6861725aa73bb286cb608c18ef8eb01f30d
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 21:44:45 2023 +0200

    Whoops, undo testing

commit 280897c9352a49947e434ba96299a89cb6672f72
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 21:23:21 2023 +0200

    Add Data Sync Client for Svelte documentation

commit c0f3dfeb929a8e23bc824a104fbcfb7253f0a7fd
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 20:37:55 2023 +0200

    Add documentation for WP JS Data Sync package

commit 5a3572f327cabde38a7de0ff65ae7546806406ae
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 20:37:35 2023 +0200

    Clarify comments a bit further

commit 0ba1672397746b19c1f61cdb7e659283d14042ff
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 20:37:03 2023 +0200

    A more consistent API to leverage WordPress

    WordPress REST API handles data via `JSON` key, this makes more similar in both directions. I would prefer this be `data` instead, but 🤷‍♂️

commit 582b952bca68ce2452a71ff3be2274e5e616624d
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 20:29:52 2023 +0200

    Add raw_get and raw_set methods

commit f8936ce5bcf8e6c855a2f4ad57e2dcfbb42a050d
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 15:23:05 2023 +0200

    Document the main class

commit 66b738a3bb2341f9cf53697d1728f36c153598e2
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 14:42:23 2023 +0200

    Rename Template -> Handler

commit 8a5b42e22b6227d0a7f45a6df39a240c9a01e978
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 14:41:43 2023 +0200

    Clarify a couple comments

commit 4df50974316fc462335813a759687b452001aa1d
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 14:41:16 2023 +0200

    Inherit comments

commit 2e69b14158c23fb2023a28c14dd89ea5338263e8
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 14:29:50 2023 +0200

    Cleanup

commit 4abf62766c0578906bbe4f56017e91de4180f5cf
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 13:59:15 2023 +0200

    Clarify the nonce

commit 3e36fd29df4d6486caf88e1e4710cc1f6e671546
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 13:19:32 2023 +0200

    Refactor nonce class a bit

commit 6f2f37f31ac700a7cde45000398f8fa338f43378
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 13:14:24 2023 +0200

    Update ignores

commit 0466c5eaf7ad459e1f998136eb440b2429c94bb9
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 13:13:24 2023 +0200

    phpcs fixes

commit 5adc70032556194f9cab2a67ab5395f4c352743b
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 13:11:43 2023 +0200

    Clarify Data Sync Template

commit cebd3435df31b4455edcaa651e8311229c38b3f2
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 13:04:04 2023 +0200

    Document storage

commit eb6f4f3e309555302cd52417307ded289aaa3535
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 12:45:35 2023 +0200

    Clean up registry

commit 593d3d4a290f46f26ff182d0e36c30cb6944e9c5
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 11:53:31 2023 +0200

    Remove duplicate ->key() calls

commit 758d289bd6d39133de8cdc0fd89fce48c11527b7
Author: Nauris Pūķis <[email protected]>
Date:   Mon Feb 6 11:46:25 2023 +0200

    Update class names

commit 8cc6b61a8a3cb69f8e8353cef4503c9f2b5d55ff
Author: Nauris Pūķis <[email protected]>
Date:   Fri Feb 3 13:41:39 2023 +0200

    phpcs: Async_Option_Template

commit beb9ccbb30f199076bff7f7cb4d416e028c1bc7d
Author: Nauris Pūķis <[email protected]>
Date:   Fri Feb 3 13:24:47 2023 +0200

    phpcs fix

commit a4f24937d9793addd0d23648ea78ec2c6df1dcfc
Author: Nauris Pūķis <[email protected]>
Date:   Fri Feb 3 13:13:54 2023 +0200

    Refactor types

commit 5051d400834379b2a1ab3ed42bf36482b7ed6457
Author: Nauris Pūķis <[email protected]>
Date:   Fri Feb 3 12:57:06 2023 +0200

    Clean-up

commit abfb98e2be21c61c77aacbfbaf225b539bfe05fd
Author: Nauris Pūķis <[email protected]>
Date:   Fri Feb 3 12:35:15 2023 +0200

    getClient() -> getPublicInterface()

commit 8a534e52d8e6d586f6397cfb6a8284728a7bbaf4
Author: Nauris Pūķis <[email protected]>
Date:   Fri Feb 3 12:34:52 2023 +0200

    Allow `validatedRequest` to throw errors as necessary

commit de3965e239ad94230bfea4dcdef5120d334df0c6
Author: Nauris Pūķis <[email protected]>
Date:   Fri Feb 3 12:34:32 2023 +0200

    Clean up client.ts and update inline docs

commit 1e8307c5b71f9772b873392e488ec76012afc9a1
Author: Nauris Pūķis <[email protected]>
Date:   Fri Feb 3 12:09:20 2023 +0200

    Refactor the synchronization process

commit 7ec72d7d86bcf8195bb61f5b7424adf5680c25ab
Author: Nauris Pūķis <[email protected]>
Date:   Thu Feb 2 16:16:59 2023 +0200

    Add sleep()

commit 052697a3579ee628fac526a6b1b8f9233ff8c6ac
Author: Nauris Pūķis <[email protected]>
Date:   Thu Feb 2 16:16:45 2023 +0200

    Break up the monolith into smaller more manageable chunks

commit 7272d6afa6e885d6b774f8e9335ee5ecebf4bf01
Author: Nauris Pūķis <[email protected]>
Date:   Wed Feb 1 16:42:36 2023 +0200

    Gracefully handle missing REST API details

commit a6f69b64e5971fc59b8236ea7c5799172ca49cce
Author: Nauris Pūķis <[email protected]>
Date:   Wed Feb 1 16:41:57 2023 +0200

    Refactor the API initialization

commit 12e337321462836e11efe2736817facf77282def
Author: Nauris Pūķis <[email protected]>
Date:   Tue Jan 31 17:42:03 2023 +0200

    Make it compile cleanly - even though it's wrong

commit b9cffca8fa26820c32639bf614192472720ad435
Author: Nauris Pūķis <[email protected]>
Date:   Tue Jan 31 17:36:18 2023 +0200

    Extract Rest API Setup

commit adf6f18530ba802a83f82afeb6e130d1269925e4
Author: Nauris Pūķis <[email protected]>
Date:   Tue Jan 31 17:32:37 2023 +0200

    Narrow types

commit 309dfc09e0f4cdb64812b0ec469d8602f7b4bc57
Author: Nauris Pūķis <[email protected]>
Date:   Tue Jan 31 17:28:47 2023 +0200

    Refacto to generalize functions, names and types

commit 6fb0ceed8afb955631d5e76f283546286a06e2a0
Author: Nauris Pūķis <[email protected]>
Date:   Tue Jan 31 16:47:44 2023 +0200

    Forced comments don't provide any value over TypeScript types

commit affe7ac3a61ae418a18587a7f07c8270264c6167
Author: Nauris Pūķis <[email protected]>
Date:   Mon Jan 30 17:11:25 2023 +0200

    AsyncAPI -> API

commit 2838ef19fa8a1d5dfdb49b67257eaec8b929d2cf
Author: Nauris Pūķis <[email protected]>
Date:   Mon Jan 30 15:39:00 2023 +0200

    Remove unused Global.ts

commit f387cfecbc334c21aacd31256086b69af211709f
Author: Nauris Pūķis <[email protected]>
Date:   Mon Jan 30 14:07:33 2023 +0200

    Address linting issues

commit bfa45fd88b935be9926df61ac36a2c9e27928dc5
Author: Nauris Pūķis <[email protected]>
Date:   Fri Jan 27 18:53:24 2023 +0200

    Use wp file naming scheme

commit fa79c637799b8bb35fea73b889934ad942f84565
Author: Nauris Pūķis <[email protected]>
Date:   Wed Jan 25 17:54:21 2023 +0200

    Make PHPCS very happy

commit 73758c8d65de75f69bb29df4d23bb140bc226d91
Author: Nauris Pūķis <[email protected]>
Date:   Tue Jan 24 09:46:59 2023 +0200

    Remove fatal errors that prevent compiling

commit 80e211703ffb0812c9816460f5dfe6c9c958c95d
Author: Nauris Pūķis <[email protected]>
Date:   Mon Jan 23 10:15:45 2023 +0200

    Update namespace

commit 263122bdeb65d63a918cd4781d5616d6c9eaf58f
Author: Nauris Pūķis <[email protected]>
Date:   Mon Jan 23 10:15:33 2023 +0200

    Update nonce name

commit 957e5a6ffc0b440eede11813f9a398497025bda8
Author: Nauris Pūķis <[email protected]>
Date:   Mon Jan 23 10:10:08 2023 +0200

    Pull in the old async-options implementation
@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

@pyronaur
Copy link
Contributor Author

pyronaur commented Feb 6, 2023

Note: I ran into a merge issue and ended up squashing the history

@pyronaur pyronaur requested review from a team and thingalon February 6, 2023 20:24
@pyronaur pyronaur added the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Feb 6, 2023
@pyronaur pyronaur marked this pull request as ready for review February 6, 2023 20:32
@pyronaur pyronaur self-assigned this Feb 6, 2023
Copy link
Member

@thingalon thingalon left a comment

Choose a reason for hiding this comment

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

For the most part, this is clean and well documented code that is easy to read.

There are a few spots where I had to go searching for answers, so I've suggested a few improvments:

Comment on lines 13 to 15
/*
* Convert `widget_name` to `widget-name` to match the endpoint.
*/
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be a docblock-style comment.

Copy link
Member

Choose a reason for hiding this comment

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

Just tagging along here - does it make sense to note how endpoints look like when they are built?

Comment on lines 21 to 26
public async request(
endpoint: string,
method: RequestMethods = 'GET',
nonce: string,
params?: RequestParams
) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we have an example endpoint in a comment? I have to read the whole class to figure out whether endpoint is a complete URL, and if not which part of the URL fragment to include.

Comment on lines +30 to +36
public POST = async ( params: T ): Promise< T > => {
return await this.validatedRequest( 'POST', params );
};

public DELETE = async () => {
return await this.validatedRequest( 'DELETE' );
};
Copy link
Member

Choose a reason for hiding this comment

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

Why are these written as class member variables that contain a function instead of as functions? (i.e.: like GET)

Copy link
Contributor Author

@pyronaur pyronaur Feb 7, 2023

Choose a reason for hiding this comment

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

So that they can be run in a callback without losing this - seems worth adding a comment

Comment on lines +7 to +11
export function getValidatedValue< T extends z.ZodSchema >(
namespace: string,
valueName: string,
valueSchema: T
): ValidatedValue< T > {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the purpose of this function is self-evident until you read setupRestApi below and the comment associated with it. I think it deserves its own comment explaining what it's for.

Comment on lines 65 to 68
* 1. Which namespace to use?
* 2. Create a Store that's going to sync.
* 3. Reference $widget in Svelte component to use it as a regular Svelte Store.
* 4. This will update the Svelte Store and POST `false` to `example.com/wp-json/jetpack-boost/widget-status`
Copy link
Member

Choose a reason for hiding this comment

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

This numbering mixes spaces and tabs, causing a janky in-and-out line-up on any tab width other than 4.

"Which namespace to use?" -- should this explain that the namespace is the name of the global/window variable that these values go in? Maybe give an example.

Why do we want to POST false to widget-status?

I like that this is followed by a js example. Maybe move these numbers into the js sample as ... comments within the comment? Is that silly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's something janky going on GitHub I think, looked fine in the editor. I've updated it and clarified it anyway.

As for the JS example, it's subtle - but each of the steps is numbered ( 1: , 2: ) and references a comment above. I prefer to keep them separate so that if you just want a code example, you can get it without comments getting in the way.

const endpoint = new API_Endpoint< z.infer< T > >( api, valueName, schema );

// Wire up store to the endpoint.
// This is customizable.
Copy link
Member

Choose a reason for hiding this comment

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

This is customizable.

I don't know what you mean by this. :)

* @param {JSONSchema | string} value - The value to stringify.
* @returns {*} - The stringified value or the original value.
*/
export function maybeStringify< T >( value: JSONSchema | string ): string | T {
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of <T>? It sounds like it will let you coerce value to be any type you want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this is some legacy code that crept in. This is only necessary for request and that's been updated to send everything as JSON anyway, so the values are always stringified now.

@thingalon
Copy link
Member

When I:

  • Add automettic/jetpack-wp-js-data-sync to the boost composer file,
  • Run composer dump-autoload and composer install
  • use use Automattic\Jetpack\WP_JS_Data_Sync\Data_Sync_Entry_Handler; at the top of my file, and
  • try to subclass it with class Boolean_Flag extends Data_Sync_Entry_Handler {

I get an error:

Class "Automattic\Jetpack\WP_JS_Data_Sync\Data_Sync_Entry_Handler" not found

Is there a step I've missed?

@pyronaur
Copy link
Contributor Author

pyronaur commented Feb 7, 2023

I think that you need to composer dump the php package and pnpm run build the js-package too. Just to be safe, jetpack build --all 😉 ?

Copy link
Member

@dilirity dilirity left a comment

Choose a reason for hiding this comment

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

I really like how structured the whole thing is - great job. I didn't set it up - just looked through the code.

@pyronaur pyronaur enabled auto-merge (squash) February 8, 2023 12:20
@pyronaur pyronaur merged commit 8911254 into trunk Feb 8, 2023
@pyronaur pyronaur deleted the boost/add/wjds-updated-v2 branch February 8, 2023 12:44
@github-actions github-actions bot removed [Status] In Progress [Status] Needs Team Review Obsolete. Use Needs Review instead. labels Feb 8, 2023
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.

5 participants