Add Fragment as a named export to React #10783

clemmy · 2017-09-22T21:46:04Z

This PR is a part of the bigger change to introduce <></> fragment syntax to JSX. With this named export, we'll be able to de-sugar <></> into React.createElement(React.Fragment) calls.

UPDATE:

Summary of discussions in PR:

We want to align fragment with array behavior.

top level keyed React.Fragment => create fragment fiber
top level un-keyed React.Fragment => no fragment fiber created
nested fragments => fragment fiber created
React.Children will treat fragments as a terminal element
In this gist (https://gist.github.com/clemmy/b3ef00f9507909429d8aa0d3ee4f986b), switching between arrays and un-keyed fragments will preserve state (e.g. Child will be preserved when rendering <>[]</> and then <><></></>)

You can think of it as the quantum theory of fragments. It has both array-like and element-like properties. 😆

acdlite

Looks good... except you wrote too many tests. How dare you!

acdlite · 2017-09-22T23:31:55Z

src/isomorphic/modern/class/__tests__/ReactFragment-test.js

+    );
+  });
+
+  it('should render via native renderer', () => {


Just the noop renderer is fine :D This doesn't touch any renderer-specific code.

Should have said so sooner! :'(

I’m sorry :( Didn’t know you were struggling with this. If something’s not renderer specific, we usually just test with the noop renderer. Except for older tests, which use React DOM.

Gotcha, good to know! :)

acdlite · 2017-09-22T23:34:10Z

Is there documentation somewhere that I should update for this PR?

Let's hold off on documentation until we're ready to release it. Also shouldn't merge this until after 16.

gaearon · 2017-09-22T23:36:15Z

With this named export, we'll be able to de-sugar <></> into React.createElement(React.Fragment)

Doesn’t it mean we’ll create two fibers for every fragment? One for the wrapper and one for the fragment itself. Seems unfortunate.

acdlite · 2017-09-22T23:37:41Z

@gaearon Yes, but I think this is fine for demo purposes and experimenting at FB. We can optimize it later.

acdlite · 2017-09-22T23:39:12Z

Although @clemmy if you want to do it the "right" way and avoid the extra fiber, I'm happy to merge that, too :D

NE-SmallTown · 2017-09-23T02:15:40Z

Relevant quip: https://fburl.com/j3hcevus

The link 404 😲

And IIRC, "<></> fragment syntax to JSX" is still under discussion, or you mean it will be accepted to JSX2.0?

acdlite · 2017-09-23T03:40:54Z

Doesn’t it mean we’ll create two fibers for every fragment

Thought this was the case but then I remembered we don’t create an extra Fragment fiber for an array returned from render. For the same reason we don’t create an extra Fragment fiber for the children of a host component. It just becomes that fiber’s child set. So, we could turn React.Fragment into a Fragment fiber instead of a FunctionalComponent fiber, but it would still just be one fiber.

acdlite · 2017-09-23T06:56:14Z

Could still be worthwhile for other reasons though

gaearon · 2017-09-23T09:25:36Z

So, we could turn React.Fragment into a Fragment fiber instead of a FunctionalComponent fiber, but it would still just be one fiber.

This is what I meant. But I agree it doesn't block this PR.

clemmy · 2017-09-25T02:12:45Z

@NE-SmallTown Oops! The link I posted basically just mentions some quirks regarding the <></> syntax such as not allowing attributes. Currently, we're experimenting with the syntax support for fragments, but not the rest of the proposals in the JSX 2.0 issue.

trueadm · 2017-09-27T21:40:32Z

I though ReactFragment would export a string of # instead of a functional component?

sophiebits · 2017-09-27T21:44:27Z

Or '#fragment'? Then to special case it we don't need to somehow detect if it's the same function, which seems treacherous.

clemmy · 2017-09-27T21:51:06Z

@trueadm I believe that's more of an implementation detail? The reason I'm pushing for this named export to be merged in ASAP is to save a bit of headache when rolling out fragment syntax changes. I'd be happy to change it to '#fragment' after.

gaearon · 2017-09-27T21:55:49Z

Would it be hard to implement '#fragment' in the scope of this PR? I'd rather not expose it as a function at all since then changing its type is technically a breaking change.

gaearon · 2017-09-27T21:57:11Z

The branching on string type happens here. We could check for '#fragment' there and create a Fiber with a private Fragment type instead that would be implemented as a function.

trueadm · 2017-09-27T21:58:46Z

@clemmy It is an implementation detail, but an important one in my opinion. Babel could instead compile to React.createElement('#fragment', ...) instead. There are a bunch of reasons for this:

We're not changing the type at later stage from function to string. Which is a breaking change.
Far easier to determine this a fragment rather than a functional component.
It's very common for React codebases to set the Babel JSX pragma to createElement and rather than import React in their module, they do import {createElement, Component} from 'React'. This would force them to go back and add Fragment.
It means the JSX has a fixed coupling with React or another pragma needs to be added to avoid this coupling.
Inferno, Preact and Vue all support JSX – using #fragment would enable those libraries to adopt fragments in the future too.

gaearon · 2017-09-27T22:00:52Z

The reason I'm pushing for this named export to be merged in ASAP is to save a bit of headache when rolling out fragment syntax changes.

Could you elaborate on this? How would the export help for the syntax? My understanding is that desugaring JSX to React.Fragment is inherently problematic because it requires a second pragma:

<>hi</>
// How do we know it's React.Fragment and not something else?
React.createElement(React.Fragment, null, 'hi')

Whereas if we use a string, we don’t have this problem.

<>hi</>
// Works with any pragma.
React.createElement('#fragment', null, 'hi')

Why is merging the export helpful in this case? I thought that, if we went with the '#fragment' proposal, we’d only need React.Fragment as a fallback for people who can’t use the syntax yet (e.g. due to waiting for tools). I don’t see why the export would be useful for the syntax itself.

aweary · 2017-09-27T22:03:33Z

cc @developit, since JSX fragments are important for other libraries that utilize JSX like Preact

acdlite · 2017-09-27T22:06:45Z

I say we merge this as-is and decide on the implementation details later. We want to start trying out the fragment syntax at Facebook ASAP.

acdlite · 2017-09-27T22:08:09Z

I like the #fragment idea but I'd also want to hear from other libraries before committing to it. So let's just go with the simplest thing for now. We can put it behind a feature flag maybe if we need to hide it from the public.

gaearon · 2017-09-27T22:08:34Z

If we only need to add it to try it at FB, we could hack it into the internal React-fb.js entry point instead. Just like we did with createClass during migration.

gaearon · 2017-09-27T22:09:35Z

But yes, either that, or let’s put it behind a feature flag. I don’t want this to become a public API before we’re truly committed to it.

acdlite · 2017-09-27T22:10:59Z

I'm fine with hacking it onto the export, too. I didn't realize we were still debating whether to export React.Fragment, only the underlying implementation.

* Add Fragment as a named export to React * Remove extra tests for Fragment * Change React.Fragment export to be a string '#fragment' * Fix fragment special case to work with 1 child * Add single child test for fragment export * Move fragment definition to ReactEntry.js and render components for key warning tests * Inline createFiberFromElementType into createFiberFromElement * Update reconciliation to special case fragments * Use same semantics as implicit childsets for ReactFragment * Add more fragment state preservation tests * Export symbol instead of string for fragments * Fix rebase breakages * Re-apply prettier at 1.2.2 * Merge branches in updateElement * Remove unnecessary check * Re-use createFiberFromFragment for fragment case * Simplyify branches by adding type field to fragment fiber * Move branching logic for fragments to broader methods when possible. * Add more tests for fragments * Address Dan's feedback * Move REACT_FRAGMENT_TYPE into __DEV__ block for DCE * Change hex representation of REACT_FRAGMENT_TYPE to follow convention * Remove unnecessary branching and isArray checks * Update test for preserving children state when keys are same * Fix updateSlot bug and add more tests * Make fragment tests more robust by using ops pattern * Update jsx element validator to allow numbers and symbols * Remove type field from fragment fiber * Fork reconcileChildFibers instead of recursing * Use ternary if condition * Revamp fragment test suite: - Add more coverage to fragment tests - Use better names - Remove useless Fragment component inside tests - Remove useless tests so that tests are more concise * Check output of renderer in fragment tests to ensure no silly business despite states being preserved * Finish implementation of fragment reconciliation with desired behavior * Add reverse render direction for fragment tests * Remove unneeded fragment branch in updateElement * Add more test cases for ReactFragment * Handle childless fragment in reconciler * Support fragment flattening in SSR * Clean up ReactPartialRenderer * Warn when non-key and children props are passed to fragments * Add non-null key check back to updateSlot's array's case * Add test for positional reconciliation in fragments * Add warning for refs in fragments with stack trace

gaearon · 2018-02-09T15:18:19Z

packages/react/src/ReactElementValidator.js

+function validateFragmentProps(fragment) {
+  currentlyValidatingElement = fragment;
+
+  for (const key of Object.keys(fragment.props)) {


We shouldn't have used this. It introduces reliance on Symbol which we don't currently require.

I think we should add a lint against for ... of as it's easy to use accidentally and it's not obvious what it compiles down to.

xyzdata · 2018-03-13T01:04:35Z

good to go!

xyzdata · 2018-03-13T01:18:10Z

I have some questions, anybody can explain it more in detail?

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

...fragments, which don’t require keys for static items:

is that mean we should use a unique key for each dynamic items?

just like the below codes shows, li no needs a unique key.

render() {
  return (
    <React.Fragment>
      <li>First item</li>
      <li>Second item</li>
      <li>Third item</li>
    </React.Fragment>
  );
}

is that mean React.Fragment will automatically add a unique key for each li item?

    <React.Fragment>
      <li key="A">First item</li>
      <li key="B">Second item</li>
      <li key="C">Third item</li>
    </React.Fragment>

clemmy · 2018-03-13T03:11:40Z

@xgqfrms-gildata No keys are required in React.Fragments because they are static in nature. However, keys are required when using the array syntax.

The wording in the documentation is a bit convoluted since the array syntax used to be called fragments. To make it clearer:

Instead of

Don't forget to add keys to elements in a fragment to avoid the key warning

It should be

Don't forget to add keys to elements in an array to avoid the key warning

xyzdata · 2018-03-13T06:00:37Z

@clemmy Thanks for your time.

good explanation.

clemmy added the Component: Core Utilities label Sep 22, 2017

clemmy requested review from flarnie and acdlite September 22, 2017 21:46

facebook-github-bot added the CLA Signed label Sep 22, 2017

acdlite requested changes Sep 22, 2017

View reviewed changes

clemmy force-pushed the react-fragment-export branch from 3e44980 to 66726c2 Compare September 23, 2017 00:53

trueadm closed this Sep 27, 2017

trueadm reopened this Sep 27, 2017

This was referenced Dec 4, 2017

Update react-dom in / from 16.1.1 to 16.2.0 dropseed/jump#28

Closed

Update react in / from 16.1.1 to 16.2.0 dropseed/jump#29

Closed

cherniavskii mentioned this pull request Dec 23, 2017

[WIP] Migration from wrapper divs to Fragments mui/material-ui-pickers#129

Merged

1 task

gaearon reviewed Feb 9, 2018

View reviewed changes

gaearon mentioned this pull request Feb 9, 2018

Disable for...of by default, rewrite cases where it matters #12198

Merged

renovate bot mentioned this pull request Feb 22, 2018

Update react monorepo packages to v16.2.0 mozilla/ensemble#28

Closed

This was referenced Mar 1, 2018

Update react-dom in / from 16.0.0 to 16.2.0 pcardune/redux-firebase-mirror#27

Open

Update react-test-renderer in / from 16.0.0 to 16.2.0 pcardune/redux-firebase-mirror#28

Open

dependabot-preview bot mentioned this pull request Mar 15, 2018

Update react requirement to 16.2.0 shankarshastri/GreenKeeperTest#4

Closed

This was referenced Mar 21, 2018

Update react to 16.3.2-abandoned sbardian/DebtTrackerAPI#15

Closed

Update react-dom to ^16.0.0-abandoned sbardian/DebtTrackerAPI#18

Closed

gaearon mentioned this pull request Nov 8, 2019

State is carried between components inside and outside of fragments #17308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Fragment as a named export to React #10783

Add Fragment as a named export to React #10783

clemmy commented Sep 22, 2017 •

edited

acdlite left a comment

acdlite Sep 22, 2017

clemmy Sep 23, 2017

acdlite Sep 23, 2017

clemmy Sep 23, 2017

acdlite commented Sep 22, 2017

gaearon commented Sep 22, 2017

acdlite commented Sep 22, 2017

acdlite commented Sep 22, 2017

NE-SmallTown commented Sep 23, 2017 •

edited

acdlite commented Sep 23, 2017

acdlite commented Sep 23, 2017

gaearon commented Sep 23, 2017

clemmy commented Sep 25, 2017

trueadm commented Sep 27, 2017

sophiebits commented Sep 27, 2017

clemmy commented Sep 27, 2017

gaearon commented Sep 27, 2017 •

edited

gaearon commented Sep 27, 2017 •

edited

trueadm commented Sep 27, 2017 •

edited

gaearon commented Sep 27, 2017

aweary commented Sep 27, 2017 •

edited

acdlite commented Sep 27, 2017

acdlite commented Sep 27, 2017

gaearon commented Sep 27, 2017

gaearon commented Sep 27, 2017

acdlite commented Sep 27, 2017

gaearon Feb 9, 2018

xyzdata commented Mar 13, 2018

xyzdata commented Mar 13, 2018 •

edited

clemmy commented Mar 13, 2018

xyzdata commented Mar 13, 2018

Add Fragment as a named export to React #10783

Add Fragment as a named export to React #10783

Conversation

clemmy commented Sep 22, 2017 • edited

Summary of discussions in PR:

acdlite left a comment

Choose a reason for hiding this comment

acdlite Sep 22, 2017

Choose a reason for hiding this comment

clemmy Sep 23, 2017

Choose a reason for hiding this comment

acdlite Sep 23, 2017

Choose a reason for hiding this comment

clemmy Sep 23, 2017

Choose a reason for hiding this comment

acdlite commented Sep 22, 2017

gaearon commented Sep 22, 2017

acdlite commented Sep 22, 2017

acdlite commented Sep 22, 2017

NE-SmallTown commented Sep 23, 2017 • edited

acdlite commented Sep 23, 2017

acdlite commented Sep 23, 2017

gaearon commented Sep 23, 2017

clemmy commented Sep 25, 2017

trueadm commented Sep 27, 2017

sophiebits commented Sep 27, 2017

clemmy commented Sep 27, 2017

gaearon commented Sep 27, 2017 • edited

gaearon commented Sep 27, 2017 • edited

trueadm commented Sep 27, 2017 • edited

gaearon commented Sep 27, 2017

aweary commented Sep 27, 2017 • edited

acdlite commented Sep 27, 2017

acdlite commented Sep 27, 2017

gaearon commented Sep 27, 2017

gaearon commented Sep 27, 2017

acdlite commented Sep 27, 2017

gaearon Feb 9, 2018

Choose a reason for hiding this comment

xyzdata commented Mar 13, 2018

xyzdata commented Mar 13, 2018 • edited

clemmy commented Mar 13, 2018

xyzdata commented Mar 13, 2018

clemmy commented Sep 22, 2017 •

edited

NE-SmallTown commented Sep 23, 2017 •

edited

gaearon commented Sep 27, 2017 •

edited

gaearon commented Sep 27, 2017 •

edited

trueadm commented Sep 27, 2017 •

edited

aweary commented Sep 27, 2017 •

edited

xyzdata commented Mar 13, 2018 •

edited