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

Extract ServerSideRender component to an independent package #15635

Conversation

@jorgefilipecosta
Copy link
Member

commented May 14, 2019

Description

ServerSideRender component was part of WordPress component. I think ServerSideRender is WordPress specific. In WordPress components, we try to keep artifacts that are generic and don't rely on WordPress API's like the rest endpoints.

In wordpress/editor we exposed a similar component that uses the WordPress components version and adds the post id as a property to be passed to the server.
Blocks that used ServerSideRender should have the wordpress/editor version because without the post id blocks, may not render as expected for that post.

WordPress/editor will not be available on the widgets screen and blocks should not rely on WordPress/editor.
This PR extracts ServerSideRender to an independent package. The extracted component checks if it is possible to query the post id from the editor store, if yes the post id is passed to the server, if not the post id is not passed to the server.

We are back-compatible with usages of wp.editor.ServerSideRender.
We are not back-compatible with usages of wp.components.ServerSideRender. To be back-compatible we would create circular reference: wp.components would depend wp.serverSideRender and wp.serverSideRender would depend on wp.components

How has this been tested?

I checked blocks using server-side render( Archives, Calendar, Lastest comments, Legacy Widgets ) still work as expected.

Show resolved Hide resolved packages/server-side-render/package.json Outdated
Show resolved Hide resolved packages/server-side-render/README.md
Show resolved Hide resolved packages/components/src/index.js
Show resolved Hide resolved packages/components/package.json
Show resolved Hide resolved packages/components/CHANGELOG.md Outdated

@jorgefilipecosta jorgefilipecosta force-pushed the update/extracted-server-side-render-to-independent-package branch from b8499c8 to 3f16f23 May 24, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

I added an inline script so we can keep back-compatibility with wp.components.ServerSideRender calls.

@jorgefilipecosta jorgefilipecosta force-pushed the update/extracted-server-side-render-to-independent-package branch 2 times, most recently from a52956b to 255aa5f May 24, 2019

Show resolved Hide resolved packages/server-side-render/README.md Outdated
Show resolved Hide resolved packages/server-side-render/README.md
}
)(
( { urlQueryArgs = EMPTY_OBJECT, currentPostId, ...props } ) => {
urlQueryArgs = useMemo( () => {

This comment has been minimized.

Copy link
@gziolo

gziolo May 27, 2019

Member

I don't think that props should be ever mutated. It might cause some unexpected behavior. In addition, it makes this code harder to follow.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta May 27, 2019

Author Member

Hi @gziolo, I think we were not mutating the props. If we do urlQueryArgs = someObject; the props the component receives stay the same as we just updated the reference to point to another object, we would be mutation the props if we did urlQueryArgs.someKey = value; I may be missing something if that's the case, please correct me, so I avoid these problems in the future. That said, I agree that the code becomes more straightforward if we use another variable, so I updated the code.

@jorgefilipecosta jorgefilipecosta force-pushed the update/extracted-server-side-render-to-independent-package branch from 022d2bd to ec10435 May 27, 2019

@jorgefilipecosta jorgefilipecosta added this to Genertic Block Editor Issues in Block Editor For Widgets May 30, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

I added a high priority label as this is a blocker to having legacy widgets on the widgets screen.

"react-native": "src/index",
"dependencies": {
"@babel/runtime": "^7.4.4",
"@wordpress/components": "file:../components",

This comment has been minimized.

Copy link
@gziolo

gziolo Jun 4, 2019

Member

@wordpress/api-fetch is missing on the list of dependencies.


export default withSelect(
( select ) => {
const coreEditorSelect = select( 'core/editor' );

This comment has been minimized.

Copy link
@gziolo

gziolo Jun 4, 2019

Member

This part is a bit concerning in my opinion. This package doesn't depend on @wordpress/editor explicitly but it will use it when it happens to be loaded on the page. While it solves the original issue of removing the dependency of core blocks on @wordpress/editor, in fact, it only hides it.

@jorgefilipecosta and @youknowriad, have you considered feeding the block editor provider with some commonly used data which is specific to the page context? In this case, it would be the current post id. In the context of widget areas, it might be necessary to know the id of the widget area. Well, it might turn out that this is the only component which would take advantage of it, so it's all academic :) I definitely don't want to block this PR but I wanted to hear your opinions.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jun 4, 2019

Author Member

Hi @gziolo, if we follow the context approach (feeding the block editor provider with some commonly used data which is specific to the page context), wouldn't the context consumer component need to be part of @wordpress/editor? In that case, we would even need to have a direct dependency on @wordpress/editor.

Based on your idea, I think a possible solution may be to put context as part of the ServerSideRender package and expose a component ServerSideRenderAdditionalDataProvider that allows to other components to pass additional props that are used in server-side render calls (the default is an empty object and no additional props are passed). The editor would use this component to pass the post id, later if the widgets also need to pass something specific it is possible to do the same.

This comment has been minimized.

Copy link
@gziolo

gziolo Jun 5, 2019

Member

Yes, ServerSideRenderProvider would solve this issue as well. I don't think there is a perfect solution to this problem. All I can think of have some drawbacks. @youknowriad will know better what is the most anticipated one 😄

Show resolved Hide resolved packages/server-side-render/README.md
@gziolo
Copy link
Member

left a comment

I still need to test this PR before I give 👍

@@ -15,6 +15,10 @@
- Fixed display of reset button when using RangeControl `allowReset` prop.
- Fixed minutes field of `DateTimePicker` missed '0' before single digit values.

### Breaking Change

This comment has been minimized.

Copy link
@gziolo

gziolo Jun 5, 2019

Member

This should be moved now to Master section. We really need to land this PR :)

Show resolved Hide resolved packages/server-side-render/package.json Outdated

export default withSelect(
( select ) => {
const coreEditorSelect = select( 'core/editor' );

This comment has been minimized.

Copy link
@gziolo

gziolo Jun 5, 2019

Member

Yes, ServerSideRenderProvider would solve this issue as well. I don't think there is a perfect solution to this problem. All I can think of have some drawbacks. @youknowriad will know better what is the most anticipated one 😄

Show resolved Hide resolved packages/server-side-render/package.json Outdated
@gziolo

gziolo approved these changes Jun 5, 2019

Copy link
Member

left a comment

It works perfectly fine. I can confirm you can still use deprecated versions of the component:
Screen Shot 2019-06-05 at 16 23 28

There is still this unresolved indirect reference to core/editor store in the code which will work but should be further discussed. See https://github.com/WordPress/gutenberg/pull/15635/files#r290156306.

If this is blocking your work, feel free to merge this PR but let's make sure this discussion continues.

jorgefilipecosta and others added some commits May 14, 2019

Update packages/server-side-render/README.md
Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>
Update packages/server-side-render/README.md
Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>
Update packages/server-side-render/README.md
Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>
Update packages/server-side-render/package.json
Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>
Update packages/server-side-render/package.json
Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>

@jorgefilipecosta jorgefilipecosta force-pushed the update/extracted-server-side-render-to-independent-package branch from ee7f705 to 532a432 Jun 5, 2019

@jorgefilipecosta jorgefilipecosta merged commit 01db600 into master Jun 5, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@jorgefilipecosta jorgefilipecosta deleted the update/extracted-server-side-render-to-independent-package branch Jun 5, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Thank you for the reviews @gziolo, I think we should follow the ServerSideRenderProvider approach, and I will gladly submit that change in a follow-up PR. @youknowriad do you have any concerns related to that approach?

@jorgefilipecosta jorgefilipecosta moved this from Generic Block Editor Issues to Done in Block Editor For Widgets Jun 5, 2019

@@ -117,3 +117,4 @@ export {
withColors,
withFontSizes,
};
export { default as ServerSideRender } from '@wordpress/server-side-render';

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 6, 2019

Contributor

Should we add a deprecated call, when using it from this location?

"\n",
array(
'( function() {',
' if ( wp && wp.components && wp.serverSideRender && ! wp.components.ServerSideRender ) {',

This comment has been minimized.

Copy link
@aduth

aduth Jun 25, 2019

Member

An issue with this is if wp-components is enqueued after wp-server-side-render, it will never be shimmed. We could potentially resolve this by defining wp-components as a dependency of wp-server-side-render (optionally checking that it's at least registered).

array(
'( function() {',
' if ( wp && wp.components && wp.serverSideRender && ! wp.components.ServerSideRender ) {',
' wp.components.ServerSideRender = wp.serverSideRender;',

This comment has been minimized.

Copy link
@aduth

aduth Jun 25, 2019

Member

If this is a component, the capitalization of serverSideRender is wrong. It should be ServerSideRender?

'( function() {',
' if ( wp && wp.components && wp.serverSideRender && ! wp.components.ServerSideRender ) {',
' wp.components.ServerSideRender = wp.serverSideRender;',
' };',

This comment has been minimized.

Copy link
@aduth

aduth Jun 25, 2019

Member

The semi-colon here is unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.