Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Server Side Render component. #5602

Merged
merged 32 commits into from
Apr 26, 2018
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
cd58776
Add blocks-renderer API endpoint. Add skeleton for ServerSideRender c…
miina Mar 12, 2018
b0ede76
Update SSR preview when editing attributes.
miina Mar 13, 2018
340136e
Fix binding preview change.
miina Mar 13, 2018
514dd4f
Fix binding preview change.
miina Mar 13, 2018
a90f40d
Add tests. Fix permission check for get_item.
miina Mar 13, 2018
0c127e1
Remove debugger line.
miina Mar 14, 2018
b0b977a
Fix some phpcs.
miina Mar 14, 2018
395a168
Fix some jscs.
miina Mar 14, 2018
1d05c2d
Show 'loading' when changing response.
miina Mar 14, 2018
c40dd25
Use wp.apiRequest for consistency.
miina Mar 14, 2018
f323651
Update ServerSideRender readme to note intended usage
westonruter Mar 14, 2018
c4abc69
Rename classes, methods, and property to be more aligned with the res…
westonruter Mar 14, 2018
6557877
Register block-renderer endpoint for each dynamic block
westonruter Mar 14, 2018
228c756
Fixes to README. Use isEqual. Renaming method.
miina Mar 16, 2018
b48ef81
Fix checking for existing response.
miina Mar 16, 2018
e39332d
Add putting together query URL from object.
miina Mar 16, 2018
46506a9
Fix some jscs.
miina Mar 16, 2018
ce1f7c6
Fix using correct props with fetching.
miina Mar 16, 2018
e0fec83
Improve putting together query URL. Use attributes as separate param.
miina Mar 17, 2018
17e404a
Make path constant more readable.
miina Mar 17, 2018
39b867a
Add setting global if is present.
miina Mar 29, 2018
4c3129b
Merge branch 'master' of github.com:WordPress/gutenberg into add/780-…
miina Apr 6, 2018
3c60d4b
Update Readme.
miina Apr 6, 2018
a10bfac
Register post_id. Add context. Fix tests.
miina Apr 16, 2018
a166324
Fix CS.
miina Apr 16, 2018
f0f4a77
Merge remote-tracking branch 'origin/master' into add/780-server-side…
miina Apr 16, 2018
6d1ee65
Add more tests. Add exception for namespace to as ruleset.
miina Apr 25, 2018
2bbbeb7
Fix eslint.
miina Apr 25, 2018
6f7d62e
Merge branch 'master' of github.com:WordPress/gutenberg into add/780-…
miina Apr 25, 2018
67af6b7
Add context param to API request.
miina Apr 25, 2018
007fb1d
Add version numbers to the phpdocs
pento Apr 26, 2018
1c9ef56
Tweak the README a little
pento Apr 26, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export { default as ResponsiveWrapper } from './responsive-wrapper';
export { default as SandBox } from './sandbox';
export { default as SelectControl } from './select-control';
export { default as Spinner } from './spinner';
export { default as ServerSideRender } from './server-side-render';
export { default as TabPanel } from './tab-panel';
export { default as TextControl } from './text-control';
export { default as TextareaControl } from './textarea-control';
Expand Down
30 changes: 30 additions & 0 deletions components/server-side-render/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
ServerSideRender
=======

ServerSideRender component is used for server-side rendering preview in Gutenberg editor, specifically for dynamic blocks. Server-side rendering in a block's `edit` function should be mostly limited for blocks which are heavily dependent on (existing) PHP rendering logic that is heavily intertwined with data, such as when there are no endpoints available.

Usage of ServerSideRender component could also be justified in the following two cases:
* Lack of potential to take some existing widget-like functionality and improve its user-facing editing experience.
* Unwillingness to create a full JS experience for a block considered legacy.

Note that ServerSideRender should be regarded as a fallback.

New blocks should be built in conjunction with any necessary REST API endpoints so that JavaScript can be used for rendering client-side in the `edit` function for the best user experience, instead of relying on using the PHP `render_callback`. The logic necessary for rendering should be included in the endpoint so that both the client-side JS and server-side PHP logic should require a mininal amount of differences.

## Usage

Render core/archives preview.
```jsx
<ServerSideRender
block="core/archives"
attributes={ this.props.attributes }
/>
```

## Output

Output is using the `render_callback` set when defining the block. For example if `block="core/archives"` as in the example then the output will match `render_callback` output of that block.

## API Endpoint
API endpoint for getting the output for ServerSideRender is `/gutenberg/v1/block-renderer/:block`. It accepts any params which are used as `attributes` for the block's `render_callback` method.

72 changes: 72 additions & 0 deletions components/server-side-render/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/**
* External dependencies.
*/
import { isEqual, isObject, map } from 'lodash';

/**
* WordPress dependencies
*/
import {
Component,
RawHTML,
} from '@wordpress/element';
import { __ } from '@wordpress/i18n';

export class ServerSideRender extends Component {
constructor( props ) {
super( props );
this.state = {
response: null,
};
}

componentDidMount() {
this.fetch( this.props );
}

componentWillReceiveProps( nextProps ) {
if ( ! isEqual( nextProps, this.props ) ) {
this.fetch( nextProps );
}
}

fetch( props ) {
this.setState( { response: null } );
Copy link
Member

Choose a reason for hiding this comment

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

We should check if this.state.response is already null here, as it will be when this function is called by componentDidMount and we'll wastefully cause an immediate re-render (setState always incurs a render unless prevented by shouldComponentUpdate).

Calling setState() in [componentDidMount] will trigger an extra rendering, but it will happen before the browser updates the screen. This guarantees that even though the render() will be called twice in this case, the user won’t see the intermediate state. Use this pattern with caution because it often causes performance issues. It can, however, be necessary for cases like modals and tooltips when you need to measure a DOM node before rendering something that depends on its size or position.

https://reactjs.org/docs/react-component.html#componentdidmount

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed within #6571.

const { block, attributes } = props;

const path = '/gutenberg/v1/block-renderer/' + block + '?' + this.getQueryUrlFromObject( { attributes } );

return wp.apiRequest( { path: path } ).then( response => {
if ( response && response.rendered ) {
Copy link
Member

Choose a reason for hiding this comment

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

There's nothing to say that the component is still mounted by the time this response is received, so attempting to setState will result in errors. If anything asynchronous is to occur in a component, it must be complemented by a cancellation in componentWillUnmount. It's a bit burdensome, but also speaks to the bigger concern that handling asynchronous behaviors in components is knowingly hard to maintain (and why extracting these behaviors out into e.g. data/core-data avoids this burden being placed on the component author and enables reuse)..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed within #6571.

this.setState( { response: response.rendered } );
}
} );
}

getQueryUrlFromObject( obj, prefix ) {
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a wordpress/url package utility.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a wordpress/url package utility.

In fact, it could probably be built-in as the default behavior for the existing addQueryArgs function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addQueryArgs works well but not for nested attributes, thus the need for creating a custom method. Couldn't find an existing utility that would put together URL in case of nested params, if you happen to know any, happy to change it!

Copy link
Member

Choose a reason for hiding this comment

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

If the behavior is needed, it could be added to the existing function via pull request to the packages repository.

return map( obj, ( paramValue, paramName ) => {
const key = prefix ? prefix + '[' + paramName + ']' : paramName,
value = obj[ paramName ];
Copy link
Member

Choose a reason for hiding this comment

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

Are paramValue and value the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed within #6571.

return isObject( paramValue ) ? this.getQueryUrlFromObject( value, key ) :
Copy link
Member

Choose a reason for hiding this comment

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

Maybe not intentional:

n_ > _.isObject( [] )
true
n_ > _.isObject( function() {} )
true

See also: https://lodash.com/docs/4.17.10#isPlainObject

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed within #6571.

encodeURIComponent( key ) + '=' + encodeURIComponent( value );
} ).join( '&' );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. The general body of this method could be simplified as:
import { map } from 'lodash';
return map( obj, ( paramValue, paramName ) => {
  const key = prefix ? ;
  return isObject( paramValue ) ? this.getQ… : encodeURI…;
} ).join( '&' );
  1. A more proper solution might be to review @wordpress/url's addQueryArgs and its choice of querystring, if nested attributes are deemed necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, replaced the method in e0fec83.
Note that I checked querystring and also query-string, these don't seem to support nested objects.


render() {
const response = this.state.response;
if ( ! response || ! response.length ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we check response.length ? Is it desirable that if an empty response was received from the server, we show a loading indicator forever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed within #6571, let me know if it looks OK now.

return (
<div key="loading" className="wp-block-embed is-loading">
Copy link
Member

Choose a reason for hiding this comment

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

For what reason do we need a key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with Placeholder within #6571.

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't reuse classes but components. If there's something in the way the embed blocks handles the loading state that is useful, it should be extracted in a separate UI component.


<p>{ __( 'Loading...' ) }</p>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need both a div and a p, or would one or the other have been sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with Placeholder within #6571.

</div>
);
}

return (
<RawHTML key="html">{ response }</RawHTML>
Copy link
Member

Choose a reason for hiding this comment

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

Should this use SandBox instead?

Copy link
Member

Choose a reason for hiding this comment

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

I guess not because then styles won't be applied.

Copy link
Contributor

@Lewiscowles1986 Lewiscowles1986 Mar 15, 2018

Choose a reason for hiding this comment

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

@westonruter Sandbox doesn't stop styles being applied (If I understand correctly it ensures no css / js bleeds into admin experience). It's used in #4710. Should work on such block-specifics defer to this once it's released?

Copy link
Member

Choose a reason for hiding this comment

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

I mean the CSS from the editor. Otherwise, the CSS has to be re-printed in the iframe.

);
}
}

export default ServerSideRender;
4 changes: 2 additions & 2 deletions lib/class-wp-block-type-registry.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ final class WP_Block_Type_Registry {
*
* @since 0.6.0
* @access private
* @var array
* @var WP_Block_Type[]
*/
private $registered_block_types = array();

Expand Down Expand Up @@ -142,7 +142,7 @@ public function get_registered( $name ) {
* @since 0.6.0
* @access public
*
* @return array Associative array of `$block_type_name => $block_type` pairs.
* @return WP_Block_Type[] Associative array of `$block_type_name => $block_type` pairs.
*/
public function get_all_registered() {
return $this->registered_block_types;
Expand Down
2 changes: 1 addition & 1 deletion lib/class-wp-block-type.php
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public function prepare_attributes_for_render( $attributes ) {
if ( isset( $attributes[ $attribute_name ] ) ) {
$is_valid = rest_validate_value_from_schema( $attributes[ $attribute_name ], $schema );
if ( ! is_wp_error( $is_valid ) ) {
$value = $attributes[ $attribute_name ];
$value = rest_sanitize_value_from_schema( $attributes[ $attribute_name ], $schema );
}
}

Expand Down
163 changes: 163 additions & 0 deletions lib/class-wp-rest-block-renderer-controller.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
<?php
/**
* Block Renderer REST API: WP_REST_Block_Renderer_Controller class
*
* @package gutenberg
* @since ?
*/

/**
* Controller which provides REST endpoint for rendering a block.
*
* @since ?
*
* @see WP_REST_Controller
*/
class WP_REST_Block_Renderer_Controller extends WP_REST_Controller {

/**
* Constructs the controller.
*
* @access public
*/
public function __construct() {
// @codingStandardsIgnoreLine - PHPCS mistakes $this->namespace for the namespace keyword.
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 ignore this in the ruleset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an exception for this file within 6d1ee65.

$this->namespace = 'gutenberg/v1';
$this->rest_base = 'block-renderer';
}

/**
* Registers the necessary REST API routes, one for each dynamic block.
*
* @access public
*/
public function register_routes() {
$block_types = WP_Block_Type_Registry::get_instance()->get_all_registered();
foreach ( $block_types as $block_type ) {
if ( ! $block_type->is_dynamic() ) {
continue;
}

// @codingStandardsIgnoreLine - PHPCS mistakes $this->namespace for the namespace keyword.
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an exception for this file within 6d1ee65.

register_rest_route( $this->namespace, '/' . $this->rest_base . '/(?P<name>' . $block_type->name . ')', array(
'args' => array(
'name' => array(
'description' => __( 'Unique registered name for the block.', 'gutenberg' ),
'type' => 'string',
),
),
array(
'methods' => WP_REST_Server::READABLE,
'callback' => array( $this, 'get_item' ),
'permission_callback' => array( $this, 'get_item_permissions_check' ),
'args' => array(
'context' => $this->get_context_param( array( 'default' => 'view' ) ),
'attributes' => array(
/* translators: %s is the name of the block */
'description' => sprintf( __( 'Attributes for %s block', 'gutenberg' ), $block_type->name ),
'type' => 'object',
'additionalProperties' => false,
'properties' => $block_type->attributes,
),
'post_id' => array(
'description' => __( 'ID of the post context.', 'gutenberg' ),
'type' => 'integer',
),
),
),
'schema' => array( $this, 'get_public_item_schema' ),
) );
}
}

/**
* Checks if a given request has access to read blocks.
*
* @since ?
* @access public
*
* @param WP_REST_Request $request Request.
* @return true|WP_Error True if the request has read access, WP_Error object otherwise.
*/
public function get_item_permissions_check( $request ) {
global $post;

$post_ID = isset( $request['post_id'] ) ? intval( $request['post_id'] ) : 0;
Copy link
Member

Choose a reason for hiding this comment

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

This optional post_id argument needs to be declared in register_rest_route()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added within a10bfac.


if ( 0 < $post_ID ) {
$post = get_post( $post_ID );
if ( ! $post || ! current_user_can( 'edit_post', $post->ID ) ) {
return new WP_Error( 'gutenberg_block_cannot_read', __( 'Sorry, you are not allowed to read Gutenberg blocks of this post', 'gutenberg' ), array(
'status' => rest_authorization_required_code(),
) );
}
} else {
if ( ! current_user_can( 'edit_posts' ) ) {
return new WP_Error( 'gutenberg_block_cannot_read', __( 'Sorry, you are not allowed to read Gutenberg blocks as this user.', 'gutenberg' ), array(
'status' => rest_authorization_required_code(),
) );
}
}

return true;
}

/**
* Returns block output from block's registered render_callback.
*
* @since ?
* @access public
*
* @param WP_REST_Request $request Full details about the request.
* @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure.
*/
public function get_item( $request ) {
global $post;

$post_ID = isset( $request['post_id'] ) ? intval( $request['post_id'] ) : 0;
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 change the casing of $post_ID to $post_id ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed within 6d1ee65.


if ( 0 < $post_ID ) {
$post = get_post( $post_ID );

// Set up postdata since this will be needed if post_id was set.
setup_postdata( $post );
}
$registry = WP_Block_Type_Registry::get_instance();
$block = $registry->get_registered( $request['name'] );
Copy link
Member

Choose a reason for hiding this comment

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

Can you return an error here if the block is invalid?


if ( null === $block ) {
return new WP_Error( 'gutenberg_block_invalid', __( 'Invalid block.', 'gutenberg' ), array(
'status' => 404,
) );
}

$data = array(
'rendered' => $block->render( $request->get_param( 'attributes' ) ),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@westonruter This isn't working with the current SSR component code since the attributes are all sent as separate params and not as one attributes array:

                const apiURL = addQueryArgs( '/gutenberg/v1/block-renderer/' + block, {
			...attributes,
			_wpnonce: wpApiSettings.nonce,
		} );

Just sending attributes: attributes as param would fail the request, is there an existing method that creates an acceptable query string for the API (e.g. ?attributes[key1]=value1&attributes[key2]=value2... etc? If not then perhaps we could still pass all the attributes as separate params and modify the endpoint accordingly. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good question. I think it's important for attributes to be under an attributes query parameter because that will prevent conflicts with block attributes. Like if a block has a name or a context these will conflict with the REST API args. It seems like this should be allowed:

const apiURL = addQueryArgs( '/gutenberg/v1/block-renderer/' + block, {
	attributes,
	_wpnonce: wpApiSettings.nonce,
} );

In other words, I think addQueryArgs needs to be able to properly convert arg values that are objects.

Copy link
Contributor Author

@miina miina Mar 16, 2018

Choose a reason for hiding this comment

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

Couldn't find an existing method at this moment, apparently query-string is intentionally not supporting nested attributes and suggests sending the object as a JSON string.

Added a custom method for now to the class to put together the query string supporting objects (ServerSideRender.getQueryUrlFromObject).

Copy link
Member

Choose a reason for hiding this comment

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

We may also be passing the full block content to the render callback in #6239

Rather than needing to remember passing all supported arguments to the render() callback, it would be useful to have a helper function for this.

);
return rest_ensure_response( $data );
}

/**
* Retrieves block's output schema, conforming to JSON Schema.
*
* @since ?
* @access public
*
* @return array Item schema data.
*/
public function get_item_schema() {
return array(
'$schema' => 'http://json-schema.org/schema#',
'title' => 'rendered-block',
'type' => 'object',
'properties' => array(
'rendered' => array(
'description' => __( 'The rendered block.', 'gutenberg' ),
'type' => 'string',
'required' => true,
Copy link
Member

Choose a reason for hiding this comment

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

context is missing from this schema attribute. Because this is only used in the editor, it should be array( 'edit' )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added within a10bfac.

'context' => array( 'edit' ),
),
),
);
}
}
1 change: 1 addition & 0 deletions lib/load.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
require dirname( __FILE__ ) . '/class-wp-block-type.php';
require dirname( __FILE__ ) . '/class-wp-block-type-registry.php';
require dirname( __FILE__ ) . '/class-wp-rest-blocks-controller.php';
require dirname( __FILE__ ) . '/class-wp-rest-block-renderer-controller.php';
require dirname( __FILE__ ) . '/blocks.php';
require dirname( __FILE__ ) . '/client-assets.php';
require dirname( __FILE__ ) . '/compat.php';
Expand Down
11 changes: 11 additions & 0 deletions lib/register.php
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,17 @@ function gutenberg_register_post_types() {
}
add_action( 'init', 'gutenberg_register_post_types' );

/**
* Registers the REST API routes needed by the Gutenberg editor.
*
* @since ?
*/
function gutenberg_register_rest_routes() {
$controller = new WP_REST_Block_Renderer_Controller();
$controller->register_routes();
}
add_action( 'rest_api_init', 'gutenberg_register_rest_routes' );

/**
* Gets revisions details for the selected post.
*
Expand Down
Loading