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

SSR: Add wp-context, wp-bind, wp-class, wp-style #133

Merged
merged 30 commits into from
Jan 26, 2023

Conversation

ockham
Copy link
Collaborator

@ockham ockham commented Jan 19, 2023

Implement Server-side rendering of those directives via WP_HTML_Tag_Processor.

Gutenberg 15.0 is the first version to include get_attribute_names_with_prefix(). This means that we now have everything we need to implement wp-bind, wp-class, and wp-style. This PR thus adds those directives, plus wp-context.

This also adds PHP unit test infrastructure and CI, in order to allow us to adopt a test-driven development approach for individual directives. Those "infrastructure" files are mostly copied directly from GB, with some minor modifications. Refer to individual commit messages to learn more.

When testing locally, note that you might have to update your wp-env environment (npm run wp-env start -- --update) to ensure that your environment is using the latest GB version.

To test locally, run npm run test:unit:php.

Based on the more experimental #125.

@ockham ockham self-assigned this Jan 19, 2023
@ockham ockham changed the title SSR: Add context and bind directives SSR: Add wp-context, wp-bind, wp-class, wp-style Jan 19, 2023
@ockham
Copy link
Collaborator Author

ockham commented Jan 19, 2023

@WordPress/frontend-dx I'd like to open this for review already -- it implements (basic versions of) a few directives and establishes some infrastructure.

@ockham ockham marked this pull request as ready for review January 19, 2023 15:23

require_once __DIR__ . '/utils.php';

function process_wp_bind( $tags, $context ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the interface I ended up with for the "directive processors".

  • $tags is a WP_HTML_Tag_Processor object
  • $context is a WP_Directive_Context (see)

I previously tried to avoid passing a "whole" WP_HTML_Tag_Processor, but it turned out that we need pretty much everything that it offers, since we:

  • Need to read its attribute values.
  • Need to be able to change its attribute values (e.g. wp-bind, wp-class, wp-style).
  • Need to be able to change the inner HTML of a given directive (e.g. wp-text and wp-html).

All that functionality is (or will be) provided by WP_HTML_Tag_Processor (or possibly a TBD WP_HTML_Processor that supports inner HTML modifications on top of tag-only modifications), so I'm pretty sure we need to pass the entire thing to the directive processors.

As for the second argument ($context): This might change, since we'll probably also want to be able to access other kind of contextual data (e.g. state), but I think it'll be easy enough to lump that together into one "context"/"state"/"data" arg later.

@@ -0,0 +1,39 @@
<?php

function get_from_context( $expr, $context ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is currently only reading object paths, based on the current $context.

E.g. if $expr is context.myblock.isOpen, and $context is array( 'myblock' => array( 'isOpen' => true ) ), then get_from_context( $expr, $context ) will indeed return true.

In the long run, we might want to support more complex expressions, e.g. context.myblock.isOpen && ! context.otherBlock.isOpen. This means we'll have to implement an expression parser.

Copy link
Member

@luisherranz luisherranz left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks @ockham 🙂👏

I've left a couple of comments below.

Comment on lines +14 to +16
if ( 'WP-BIND' === $tags->get_tag() ) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it'd be better to divide the definition of directive attributes and directive tags into different functions as we do in JavaScript. Something like process_wp_bind_tag and process_wp_bind_attribute.

What do think, @ockham?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I think that might make sense 🤔 Some directives even only make sense as either attribute (e.g. wp-bind, wp-class, and wp-style) or tag, but not as both. Keeping track of the context stack separately for wp-context tags and attributes also makes sense.

Originally, I liked the simplicity of having a map of directive names and corresponding process_wp_* functions, but that's not really a strong counterargument, so I think I'll go with separate processor functions for tags and attributes 👍

(I'd like to do that in a follow-up to this PR if that's okay 😊 )

Copy link
Member

Choose a reason for hiding this comment

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

(I'd like to do that in a follow-up to this PR if that's okay 😊

Oh, don't worry too much about it, we can define the API to create directives later, and we will try to match the JS and PHP functions. Not important. I just wanted to know about your reasoning.

Keeping track of the context stack separately for wp-context tags and attributes also makes sense.

What do you mean by this?

sorry for my late reply!

} else {
// Components can't have directives (unless we change our mind about this).
foreach ( $directives as $directive => $directive_processor ) {
$attributes = $tags->get_attribute_names_with_prefix( $directive );
Copy link
Member

Choose a reason for hiding this comment

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

I wonder what would be more performant, doing get_attribute_names_with_prefix once per directive attribute, or just doing it once to get all the attributes and search for the directives ourselves.

Have you thought about it, @ockham?

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 hadn't before, but I like the idea! Happy to work on that (in another follow-up 😊)

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 want to be a doomsayer, but wouldn't doing that defeat the purpose of introducing the with_prefix in the first place? 😆

Copy link
Member

Choose a reason for hiding this comment

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

Happy to work on that (in another follow-up 😊)

By the way, I don't know the internals of get_attribute_names_with_prefix. I was just wondering which method would be more performant. I don't know if getting all the attributes at once is.

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.

None yet

2 participants