Extract ServerSideRender component to an independent package by jorgefilipecosta · Pull Request #15635 · WordPress/gutenberg

jorgefilipecosta · 2019-05-14T18:01:20Z

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.

packages/server-side-render/package.json

packages/server-side-render/README.md

packages/components/src/index.js

packages/components/package.json

packages/components/CHANGELOG.md

jorgefilipecosta · 2019-05-24T17:52:38Z

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

packages/server-side-render/README.md

gziolo · 2019-05-27T06:56:06Z

packages/server-side-render/src/index.js

+	}
+)(
+	( { urlQueryArgs = EMPTY_OBJECT, currentPostId, ...props } ) => {
+		urlQueryArgs = useMemo( () => {


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.

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 · 2019-06-03T17:10:07Z

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

gziolo · 2019-06-04T06:49:27Z

packages/server-side-render/package.json

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


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

gziolo · 2019-06-04T07:16:34Z

packages/server-side-render/src/index.js

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


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.

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.

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 😄

packages/server-side-render/README.md

gziolo

I still need to test this PR before I give 👍

gziolo · 2019-06-05T12:37:07Z

packages/components/CHANGELOG.md

 - Fixed display of reset button when using RangeControl `allowReset` prop.
 - Fixed minutes field of `DateTimePicker` missed '0' before single digit values.

+### Breaking Change


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

packages/server-side-render/package.json

gziolo · 2019-06-05T12:45:16Z

packages/server-side-render/src/index.js

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


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 😄

packages/server-side-render/package.json

gziolo

It works perfectly fine. I can confirm you can still use deprecated versions of the component:

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.

Co-Authored-By: Grzegorz (Greg) Ziółkowski <[email protected]>

jorgefilipecosta · 2019-06-05T15:57:20Z

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?

youknowriad · 2019-06-06T10:24:17Z

packages/editor/src/components/deprecated.js

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


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

I just ran into this while writing a plugin. Because wp.components.ServerSideRender is available with no deprecation warnings, it's easy to think that this is still the canonical place for that component.

The other thing is that the wp-components module hasn't listed wp-server-side-render as a dependency, so as a plugin author I have to know that my plugin depends on wp-server-side-render on top of the rest. Finally, we are in a strange situation now because those two modules are interdependent. I believe that, going forward, the only proper solution is to remove ServerSideRender from components's deprecated components.

Of course, before that is possible, we should at least wrap wp.components.ServerSideRender so that it logs a deprecation warning.

Ah, this line was about wp-editor and I missed that. I guess it speaks to the general confusion. :) Never mind the bit about mutual dependencies, then.

aduth · 2019-06-25T17:12:57Z

lib/client-assets.php

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


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).

aduth · 2019-06-25T17:13:14Z

lib/client-assets.php

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


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

aduth · 2019-06-25T17:13:33Z

lib/client-assets.php

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


The semi-colon here is unnecessary.

jorgefilipecosta requested review from Soean, aduth, ajitbohra, chrisvanpatten, gziolo, mkaz, noisysocks, notnownikki, talldan and youknowriad as code owners May 14, 2019 18:01

jorgefilipecosta mentioned this pull request May 15, 2019

Widget-block Screen in wp-admin #13204

Closed

gziolo reviewed May 15, 2019

View reviewed changes

gziolo added npm Packages Related to npm packages [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality labels May 15, 2019

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

jorgefilipecosta added the [Feature] Widgets Screen The block-based screen that replaced widgets.php. label May 24, 2019

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

gziolo reviewed May 27, 2019

View reviewed changes

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

jorgefilipecosta added Needs Technical Feedback Needs testing from a developer perspective. [Priority] High Used to indicate top priority items that need quick attention labels May 30, 2019

jorgefilipecosta mentioned this pull request Jun 3, 2019

Add reference widgets to legacy widget blocks. Use ajax admin endpoint on this blocks. #15801

Merged

gziolo reviewed Jun 4, 2019

View reviewed changes

gziolo reviewed Jun 5, 2019

View reviewed changes

gziolo removed the Needs Technical Feedback Needs testing from a developer perspective. label Jun 5, 2019

gziolo approved these changes Jun 5, 2019

View reviewed changes

jorgefilipecosta and others added 7 commits June 5, 2019 16:17

Extract ServerSideRender component to an independent package

f579af4

Update packages/server-side-render/README.md

c062f23

Co-Authored-By: Grzegorz (Greg) Ziółkowski <[email protected]>

Update packages/server-side-render/README.md

d468439

Co-Authored-By: Grzegorz (Greg) Ziółkowski <[email protected]>

Use another variable for urlQueryArgs.

104cbae

Update packages/server-side-render/README.md

4782648

Co-Authored-By: Grzegorz (Greg) Ziółkowski <[email protected]>

Update packages/server-side-render/package.json

5882bd6

Co-Authored-By: Grzegorz (Greg) Ziółkowski <[email protected]>

Update packages/server-side-render/package.json

532a432

Co-Authored-By: Grzegorz (Greg) Ziółkowski <[email protected]>

jorgefilipecosta force-pushed the update/extracted-server-side-render-to-independent-package branch from ee7f705 to 532a432 Compare June 5, 2019 15:34

jorgefilipecosta merged commit 01db600 into master Jun 5, 2019

jorgefilipecosta deleted the update/extracted-server-side-render-to-independent-package branch June 5, 2019 15:56

youknowriad reviewed Jun 6, 2019

View reviewed changes

youknowriad added this to the Gutenberg 5.9 milestone Jun 7, 2019

youknowriad added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jun 7, 2019

jorgefilipecosta mentioned this pull request Jun 12, 2019

Add deprecated call to wp.components.ServerSideRender; Remove some missed wp.components.ServerSideRender usages. #16133

Merged

aduth reviewed Jun 25, 2019

View reviewed changes

youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Sep 23, 2019

Soean mentioned this pull request Dec 9, 2019

Docs: Update ServerSideRender import #19012

Merged

Conversation

jorgefilipecosta commented May 14, 2019

Description

How has this been tested?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jorgefilipecosta commented May 24, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgefilipecosta commented Jun 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gziolo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgefilipecosta commented Jun 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcsf Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gziolo left a comment •

edited

Loading

mcsf Mar 23, 2020 •

edited

Loading