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

Editor: Deprecate ServerSideRender in favour of @wordpress/server-side-render #21081

Closed
wants to merge 1 commit into from

Conversation

mcsf
Copy link
Contributor

@mcsf mcsf commented Mar 23, 2020

Description

Use of component wp.editor.ServerSideRender now raises a deprecation warning recommending wp.serverSideRender as an alternative.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@mcsf mcsf added Backwards Compatibility Issues or PRs that impact backwards compatability [Package] Server Side Render /packages/server-side-render [Package] Editor /packages/editor labels Mar 23, 2020
@github-actions
Copy link

Size Change: +24 B (0%)

Total Size: 857 kB

Filename Size Change
build/edit-post/index.js 91.2 kB -1 B
build/edit-widgets/index.js 4.43 kB -2 B (0%)
build/editor/index.js 43.8 kB +27 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 998 B 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 100 kB 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 7.24 kB 0 B
build/block-library/editor.css 7.24 kB 0 B
build/block-library/index.js 110 kB 0 B
build/block-library/style-rtl.css 7.41 kB 0 B
build/block-library/style.css 7.42 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.5 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.2 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/style-rtl.css 8.47 kB 0 B
build/edit-post/style.css 8.46 kB 0 B
build/edit-site/index.js 5.95 kB 0 B
build/edit-site/style-rtl.css 2.69 kB 0 B
build/edit-site/style.css 2.69 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/style-rtl.css 4 kB 0 B
build/editor/style.css 3.98 kB 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 6.95 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.69 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 781 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.4 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@aduth
Copy link
Member

aduth commented Mar 23, 2020

I'm wondering if this is seeking to update the canonical reference to the component, if we should first resolve the current issue of its naming. Since it is a React component, it would be expected to have the casing wp.ServerSideRender.

Previously:

@mcsf
Copy link
Contributor Author

mcsf commented Mar 24, 2020

I'm wondering if this is seeking to update the canonical reference to the component, if we should first resolve the current issue of its naming. Since it is a React component, it would be expected to have the casing wp.ServerSideRender.

However verbose, what's wrong with wp.serverSideRender.ServerSideRender? Not only is that more practical to implement, since it impacts only the existing module, but in my mind it's also the most correct: it's the component provided by the module (which can be thought of as a namespace too). It just so happens that the module is very specific and bears the same name as the component.

@aduth
Copy link
Member

aduth commented Mar 24, 2020

@mcsf I don't have strong feelings against it, but in addition to being more verbose, there's a few other details to note:

  • This isn't the only package that behaves this way (source).
  • We still need some system to "override" the default camel-case behavior where manual intervention is necessary. For example, wp.escapeHtml should be wp.escapeHTML per naming conventions, even though it is not one of these packages whose default export is assigned directly to the wp global.

Verbosity is likely more relevant for ES5 usage. In ES2015+, it would amount to the difference of a pair of squiggly brackets:

import { ServerSideRender } from '@wordpress/server-side-render';
import ServerSideRender from '@wordpress/server-side-render';

@mcsf
Copy link
Contributor Author

mcsf commented Mar 24, 2020

Yes, all of those are good points. It seems we have bigger fish to fry then, and I'd rather not have such an unimportant PR as this one linger because of a pending decision. I'll close this.

The broader topic seems like something for the Core JS group, don't you think?

@mcsf mcsf closed this Mar 24, 2020
@aduth
Copy link
Member

aduth commented Mar 24, 2020

It's been discussed a few times, both in and out of the weekly meetings:

The sense from the conversation in 2018 was that it wasn't too important to be worrying over, or at least there wasn't a good convention for deprecations. Based on some of my later suggestions, I think we can still do a deprecation exactly like what was proposed in this pull request, but as part of an additional effort to introduce the implementation to assign the globals correctly.

There's a JavaScript meeting a few minutes from now, I can plan to bring it up again there. I sense at this point it's more a matter of (a) getting an issue to track the effort and (b) actually doing it.

@aduth
Copy link
Member

aduth commented Mar 24, 2020

Discussed during today's JavaScript chat (link requires registration):

https://wordpress.slack.com/archives/C5UNMSU4R/p1585059454306400

Action items:

  • Merge your pull request (I’ll approve it)
  • Create an issue to track the broader effort of “fixing” the names for these globals

@aduth aduth deleted the fix/server-side-render-add-deprecation branch March 24, 2020 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Package] Editor /packages/editor [Package] Server Side Render /packages/server-side-render
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants