Make WordPress Core

Opened 3 months ago

Closed 8 weeks ago

Last modified 8 weeks ago

#61037 closed feature request (fixed)

Interactivity API: Directives cannot derive state on the server

Reported by: jonsurrell's profile jonsurrell Owned by: jonsurrell's profile jonsurrell
Milestone: 6.6 Priority: normal
Severity: normal Version: 6.5
Component: Interactivity API Keywords: has-patch has-unit-tests
Focuses: javascript Cc:

Description

The Interactivity API has a concept of "derived state" but it only works on the client (JavaScript). There is no analogous on the server, so derived state does not have a good server-side solution.

The client side implementation uses getter functions.

There are some workarounds on the server such as calling wp_interactivity_state to set the "derived" value in state. There are some situations, such as in a wp-each directive, where it appears there is no way to use derived state on the server and always fallback to client-side rendering.

Change History (19)

This ticket was mentioned in PR #6394 on WordPress/wordpress-develop by @jonsurrell.


3 months ago
#1

  • Keywords has-patch has-unit-tests added

Implement derived state callback handling in server directive processing.

The implementation expects a Closure type. call_user_func was avoided because it would be difficult to differentiate the string paths in the directive from a callable function name.

Trac ticket: https://core.trac.wordpress.org/ticket/61037

#2 @fabiankaegy
3 months ago

  • Milestone changed from Awaiting Review to 6.6

@luisherranz commented on PR #6394:


3 months ago
#3

Thanks for working on this, Jon 🙂

I think function closures is the way to go!

We had them implemented exactly as you've done here before, but we removed them to have more time to evaluate them before its public release.

However, what about mimicking the client store and, instead of this:

wp_interactivity_state( 'myPlugin', array(
  'list' => array(1, 2, 3),
  'text' => __('this is'),
  'itemText' => function( $store ) {
    return $store['state']['text'] . ': ' . $store['context']['item'];
  }
));

Do this:

$state = wp_interactivity_state( 'myPlugin', array(
  'list' => array(1, 2, 3),
  'text' => __('this is'),
  'itemText' => function() use ($state) {
    $context = wp_interactivity_get_context( 'myPlugin' );
    return $state['text'] . ': ' . $context['item'];
  }
));

That way, it'd work with the "same API" in the client and in the server.

What do you think?

@darerodz commented on PR #6394:


3 months ago
#4

That way, it'd work with the "same API" in the client and in the server.

What do you think?

I think that would be nice. 😄

Just a note: for both approaches, if you want to read from a getter, I guess you would have to execute it, right? And it's the same for both approaches, with the difference that, for the second case, you don't have to pass any arguments. Following the example above, let's imagine that state.text is also a getter:

Approach 1

wp_interactivity_state( 'myPlugin', array(
  'list' => array(1, 2, 3),
  'text' => function ( $store ) {
    return __( $store['context']['text'] );
  },
  'itemText' => function ( $store ) {
    return $store[ $store  'state']['text'] . ': ' . $store['context']['item'];
  }
));

Approach 2

$state = wp_interactivity_state( 'myPlugin', array(
  'list' => array(1, 2, 3),
  'text' => function () {
    $context = wp_interactivity_get_context( 'myPlugin' );
    return __( $store['context']['text'] );
  },
  'itemText' => function () use ( $state ) {
    $context = wp_interactivity_get_context( 'myPlugin' );
    return $state['text']() . ': ' . $context['item'];
  }
));

I see a benefit with the second approach: you don't need a reference to the $store to read the value from a state getter. 🤔

@jonsurrell commented on PR #6394:


3 months ago
#5

I don't think it's viable to use a closure over the $state variable. That's effectively a snapshot of the state, but I think we should have access to the full state when the callback runs. I believe this aligns with what we have on the client.

Consider the following:

$s1 = wp_interactivity_state( 'ns', array( 'x'=>'y' ) );
$s2 = wp_interactivity_state( 'ns', array( 'z'=>'2' ) );

var_dump($s1, $s2, $s1 === $s2);

This prints:

array(1) {
  ["x"]=>
  string(1) "y"
}
array(2) {
  ["x"]=>
  string(1) "y"
  ["z"]=>
  string(1) "2"
}
bool(false)

The state changes and $s1 is out of date. Our closure might see that (depending on subsequent state changes). When the callback is evaluated, it should access the up-to-date current state at that moment.

If we were to use closures, we'd probably have to resort to some ugly reference passing that I don't think is a good idea.

@luisherranz commented on PR #6394:


3 months ago
#6

What about this?

wp_interactivity_state( 'myPlugin', array(
  'list' => array(1, 2, 3),
  'text' => __('this is'),
  'itemText' => function() use ($state) {
    $state   = wp_interactivity_state( 'myPlugin' );
    $context = wp_interactivity_get_context( 'myPlugin' );
    return $state['text'] . ': ' . $context['item'];
  }
));

@jonsurrell commented on PR #6394:


3 months ago
#7

That seems like it would work, but what's the advantage? Is there a drawback to passing in the state and context as parameters? We'd have to implement the context function which doesn't exist right now as far as I know. That function would also only make sense when called in one of these functions while processing directives, right?

@luisherranz commented on PR #6394:


3 months ago
#8

Is there a drawback to passing in the state and context as parameters?

I see a few drawbacks/inconsistencies.

  1. In the client, the context is not part of the store, so store['context'] in the server would be inconsistent.
  2. In the server, stores don't exist per se, only the initial state of those client stores, so store itself doesn't make much sense (as opposed to accessing state directly).
  3. In the client, getters don't get the store via arguments.
  4. In the client you can access other namespaces passing the namespace to the functions (store( 'otherPlugin' ) or getContext( 'otherPlugin')). If we use arguments, it's not clear how to get the state or context from a different namespace.

I agree that using functions to get the state and context feels boilerplaty (especially with WordPress' long function names), but it would be the way to make it more consistent with the client experience.

By the way, as those server "getters" are synchronous, maybe we can populate the default namespace and omit it, like we do in the client:

wp_interactivity_state( 'myPlugin', array(
  'list' => array(1, 2, 3),
  'text' => __('this is'),
  'itemText' => function() {
    $state   = wp_interactivity_state();
    $context = wp_interactivity_get_context();
    return $state['text'] . ': ' . $context['item'];
  }
));

#9 @jonsurrell
3 months ago

  • Component changed from General to Interactivity API

#10 @gziolo
3 months ago

  • Version set to 6.5

@darerodz commented on PR #6394:


2 months ago
#11

A couple of notes after playing a bit with this PR, trying to follow @luisherranz's approach:

  • The functions to access the state and the context inside getters (wp_interactivity_state and wp_interactivity_get_context) are global.
  • They have access to the global WP_Interactivity_API instance.
  • They depend on the current state, context and namespace stacks during a process_directives() call.
  • The state is available from a WP_Interactivity_API instance, but the context and namespace aren't. Instead, they're local to the process_directives() method and passed down to other methods as arguments (see this).
  • That's what we're trying to avoid in this case... 🤔

To solve this, I guess we need to move the context and namespace stacks to class properties. @sirreal, do you agree with this approach? Any concerns or different ideas?

---

PS: if we do this, I imagine that would require updating a bunch of PHPUnit tests... 😬

@jonsurrell commented on PR #6394:


2 months ago
#12

Yes, I think that makes sense because those global functions need to access those things.

@darerodz commented on PR #6394:


2 months ago
#13

@sirreal I think I've addressed all your suggestions. Could you take another quick look?

If everything is OK, I guess the PR is ready to be merged. cc @gziolo.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


2 months ago

#15 @oglekler
2 months ago

We have just a few days before Beta 1, so, place move this forward :)
And if you need testing, please provide testing instructions and mark the ticket accordingly.

Thank you!

#16 @jonsurrell
2 months ago

@darerodz has taken over the PR and this ticket.

@gziolo commented on PR #6394:


8 weeks ago
#17

@sirreal or @DAreRodz, can you rebase this PR so I could commit it?

#18 @gziolo
8 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 58327:

Interactivity API: Directives cannot derive state on the server

The Interactivity API has a concept of "derived state" but it only worked on the client (JavaScript). This is the implementation that mirrors it, so derived state has good server-side solution.

Props jonsurrell, darerodz, gziolo, luisherranz, cbravobernal.
Fixes #61037.

Note: See TracTickets for help on using tickets.