Framework: Remove react-addons-create-fragment #19105

ockham · 2017-10-24T13:11:06Z

That addon contains the createFragment function, which does the following: given an object of components, e.g.

    children = createFragment({
      left: props.leftChildren,
      right: props.rightChildren
    });

It adds the object keys as React keys to the components, and spits out an array consisting of these components. So I think its main use case is around working with arrays of components, which in turn is mostly relevant for ordering of sets of somewhat dynamically generated components, see https://reactjs.org/docs/create-fragment.html.

The fix is to add keys directly to those components. Easy example: https://github.com/Automattic/wp-calypso/pull/19105/files#diff-0e2c0c04cc568a7b0c3c3cb7c66b01c9

However, this can be tricky in some cases -- e.g. when we iterate over a list of inputs and want to produce a list of components, but there are more than just one component per input. Wrap in a div you say? <script /> tags in <head /> I say.

To test: Check the relevant occurrences -- they should still work.

Pages: Trash/Delete button in popover menu:

Editor Drawer: Categories

Editor Drawer: Taxonomies (example: Portfolio CPT with 'project' specific taxonomies)

Media Modal: Dimensions ('5184 ✕ 3456' in the example screenshot)

Media Library: Notices

Try e.g. uploading a video file to a site that doesn't have a plan that includes VideoPress.

`lib/embed-frame-markup/index.jsx`

This is the trickiest to test. It's used by TinyMCE to display embeds. Try embedding something...

matticbot · 2017-10-24T13:11:06Z

Test live: https://calypso.live/?branch=remove/react-addons-create-fragment

aduth · 2017-10-24T14:11:27Z

client/lib/embed-frame-markup/index.jsx

-					} )
-				) }
+				{ map( scripts, ( { extra, src }, key ) => {
+					return [


In this case we're mapping to create a nested array, e.g. [ [ <script /> ] ] . I think effectively there is no difference, specifically for how we're using it for rendering to string here, but in normal React renders I've observed to have abysmal performance characteristics (e.g. WordPress/gutenberg#3008 (comment)).

Hmm, I guess I could use flatMap instead...

aduth

Needs shrinkwrap update?

aduth · 2017-10-26T02:17:15Z

client/my-sites/pages/page/index.js

-			separator: <MenuSeparator />,
-			item: (
+		return (
+			<div>


Is there any advantage to adding a wrapping div over returning an array and specifying keys for each element? Seems like it could be nice to avoid an unnecessary DOM element.

Yeah, fair enough. I was trying to avoid arrays altogether, but I guess there's no harm in keeping them.

ockham · 2017-10-26T19:27:22Z

Needs shrinkwrap update?

D'oh, thanks. Amended the Framework: Remove react-addons-create-fragment commit to include it.

aduth · 2017-10-26T19:40:51Z

Amended the Framework: Remove react-addons-create-fragment commit to include it.

Still seeing react-addons-create-fragment in shrinkwrap on my end.

aduth · 2017-10-26T19:41:29Z

client/post-editor/media-modal/detail/detail-file-info.jsx

@@ -35,19 +34,17 @@ class EditorMediaModalDetailFileInfo extends React.Component {
 				break;

 			case 'dimensions':
-				value = createFragment( {
-					width: (
+				value = (


Could be array too. Not a big deal, but potential for styling or performance implications (by DOM node count) with wrappers.

Will that work with the ' ✕ ' string? Oh well, I'll try and find out.

Will that work with the ' ✕ ' string? Oh well, I'll try and find out.

I believe it handles strings fine without the key.

Yep, still shows up fine, and no console warnings. 34f16c7

ockham · 2017-10-26T19:53:39Z

Still seeing react-addons-create-fragment in shrinkwrap on my end.

Could be another dependency's fault.

aduth · 2017-10-26T20:06:31Z

Not sure why, but I'm seeing the trash button in Page list menu not extending to occupy the full width of the popover, and not seeing the same in production:

ockham · 2017-10-26T20:14:07Z

Hmm. Looks fine here (I switched to English to be sure):

What about that little 'Private' indicator in your popover menu? Is that supposed to be there? Cause my screenshot was also taken on a private site, and I don't see anything like that...

ockham · 2017-10-26T20:17:39Z

Oh, so that's a private page 🤦‍♂️

Anyway:

aduth

Was running a stale version of the branch. All good now 👍

ockham · 2017-10-26T20:25:49Z

Thanks a lot for reviewing @aduth!

sirreal · 2017-10-27T07:45:43Z

❤️ nicely done!

ockham self-assigned this Oct 24, 2017

ockham added the [Status] In Progress label Oct 24, 2017

ockham requested review from jsnmoon and samouri October 24, 2017 13:12

ockham added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Oct 24, 2017

ockham requested review from jsnajdr, sirreal and aduth October 24, 2017 14:00

aduth reviewed Oct 24, 2017

View reviewed changes

ockham force-pushed the remove/react-addons-create-fragment branch from 30f9189 to e9b212b Compare October 25, 2017 10:07

samouri mentioned this pull request Oct 25, 2017

Framework: update to React 16 #19083

Merged

10 tasks

aduth reviewed Oct 26, 2017

View reviewed changes

ockham force-pushed the remove/react-addons-create-fragment branch from f015806 to 122cba4 Compare October 26, 2017 19:26

ockham added 7 commits October 26, 2017 21:35

embed-frame-markup: Remove react-addons-create-fragment

be7721a

Media Library: Drop react-addons-create-fragment

eb0780e

Editor: Drop react-addons-create-fragment from EditorDrawer

6f508af

Pages: Drop react-addons-create-fragment from Page

505fdcc

Media Modal: Remove react-addons-create-fragment from Detail Info

f3af14f

Use flatMap

c3cf6cc

Drop wrapping divs, use array and keys

3fb6b90

aduth reviewed Oct 26, 2017

View reviewed changes

Framework: Remove react-addons-create-fragment

b30700e

ockham force-pushed the remove/react-addons-create-fragment branch from 122cba4 to b30700e Compare October 26, 2017 19:43

MediaModal: Drop wrapping span, use array and keys

34f16c7

aduth approved these changes Oct 26, 2017

View reviewed changes

aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 26, 2017

ockham merged commit 5d6787d into master Oct 26, 2017

ockham deleted the remove/react-addons-create-fragment branch October 26, 2017 20:27

matticbot removed the [Status] Ready to Merge label Oct 26, 2017

aduth mentioned this pull request Oct 26, 2017

Performance: Use Lodash flatMap for accumulating blocks in block list WordPress/gutenberg#3185

Merged

Framework: Remove react-addons-create-fragment #19105

Framework: Remove react-addons-create-fragment #19105

Uh oh!

Conversation

ockham commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To test: Check the relevant occurrences -- they should still work.

Pages: Trash/Delete button in popover menu:

Editor Drawer: Categories

Editor Drawer: Taxonomies (example: Portfolio CPT with 'project' specific taxonomies)

Media Modal: Dimensions ('5184 ✕ 3456' in the example screenshot)

Media Library: Notices

lib/embed-frame-markup/index.jsx

Uh oh!

matticbot commented Oct 24, 2017

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!

aduth left a comment

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!

ockham commented Oct 26, 2017

Uh oh!

aduth commented Oct 26, 2017

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!

ockham commented Oct 26, 2017

Uh oh!

aduth commented Oct 26, 2017

Uh oh!

ockham commented Oct 26, 2017

Uh oh!

ockham commented Oct 26, 2017

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

ockham commented Oct 26, 2017

Uh oh!

sirreal commented Oct 27, 2017

Uh oh!

Uh oh!

ockham commented Oct 24, 2017 •

edited

Loading

`lib/embed-frame-markup/index.jsx`