Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

Serialize store in PHP #147

Merged
merged 25 commits into from
Feb 9, 2023
Merged

Serialize store in PHP #147

merged 25 commits into from
Feb 9, 2023

Conversation

DAreRodz
Copy link
Collaborator

@DAreRodz DAreRodz commented Feb 3, 2023

What

Adds store serialization and hydration.

Why

The framework requires them. Developers should be able to send the initial state from the server as part of the SSR.

How

  • Implementing a WP_Directive_Store class that contains the generated state.
  • Adding a store( $data ) function to define the state on the block's render callback.
  • Adding a evaluate( $path, $context ) function to access both the state and the context (like in JS)
  • Serializing and sending the store's content to the client in a <script type="application/json"> tag.
  • Parsing the serialized store in the client and using it to initialize the store.
  • Rename wpx to store

@DAreRodz DAreRodz self-assigned this Feb 7, 2023
@DAreRodz DAreRodz marked this pull request as ready for review February 7, 2023 19:03
'state' => array(
'core' => array(
'a' => 1,
'b' => 2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about throwing in a different type value (to make sure evaluate gets the type right)? 😄

Suggested change
'b' => 2,
'b' => true,

(and then assertTrue further down).

Comment on lines 36 to 41
'core' => array(
'a' => 1,
'b' => 2,
'nested' => array(
'c' => 3,
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe use a value for $context that's different from $state?

@@ -17,7 +17,7 @@ function process_wp_bind( $tags, $context ) {

// TODO: Properly parse $value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's maybe remove the TODO already -- it doesn't make sense to have it in every directive processor's individual code 😄

Suggested change
// TODO: Properly parse $value.

@@ -17,7 +17,7 @@ function process_wp_class( $tags, $context ) {

// TODO: Properly parse $value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as https://github.com/WordPress/block-hydration-experiments/pull/147/files#r1101622012

Suggested change
// TODO: Properly parse $value.

@@ -17,7 +17,7 @@ function process_wp_style( $tags, $context ) {

// TODO: Properly parse $value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as https://github.com/WordPress/block-hydration-experiments/pull/147/files#r1101622012

Suggested change
// TODO: Properly parse $value.

WP_Directive_Store::merge_data( $data );
}

function evaluate( string $path, array $context = array() ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's maybe add a TODO here.

Suggested change
function evaluate( string $path, array $context = array() ) {
// TODO: Implement evaluation of complex logical expressions.
function evaluate( string $path, array $context = array() ) {


// TODO: check if priority 9 is enough.
// TODO: check if `wp_footer` is the most appropriate hook.
add_action( 'wp_footer', WP_Directive_Store::render, 9 );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, don't we normally use a different notation for invoking a class method? 🤔

Suggested change
add_action( 'wp_footer', WP_Directive_Store::render, 9 );
add_action( 'wp_footer', array( 'WP_Directive_Store', 'render' ), 9 );

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was sure there was a syntax for that, but I couldn't remember. 😄

await expect(double).toHaveText('6');
await expect(clicks).toHaveText('0');

page.getByTestId('counter button').click();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also add unit test coverage for this functionality in the future (to test things in a more isolated way)? 😊

I.e. for wp-on:click, we'd likely want to pass a mock function and would check if it's really been called.

Or for wp-bind, we'd probably want to update a state value manually, and verify that the bound attribute is changed accordingly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. We could do so in a future PR (also for other directives/functionalities). ☝️

);
}

public function test_store_should_be_correctly_render() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Grammar nitpick 😊

Suggested change
public function test_store_should_be_correctly_render() {
public function test_store_should_be_correctly_rendered() {

Copy link
Collaborator

@ockham ockham left a comment

Choose a reason for hiding this comment

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

I left a few minor notes, but LGTM in general! 🚢

@luisherranz
Copy link
Member

Great work, David 🙂


For context here, we also need to merge the state that comes in the new page during client-side navigations.

I've been talking with David and we think merging both states should be enough. @DAreRodz can you open a new issue for that, and another one to also merge contexts?

@SantosGuillamot
Copy link
Contributor

For context here, we also need to merge the state that comes in the new page during client-side navigations.

FYI: I think we will face this issue in the Movies Demo. There will probably be interactive blocks that aren't on the homepage, and after navigating to a movie, they won't work as expected. I'm thinking of a Tabs block to toggle between images and videos, for example.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants