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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Fragment as a named export to React #10783

Merged
merged 46 commits into from Oct 31, 2017

Conversation

@clemmy
Contributor

clemmy commented Sep 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Sep 22, 2017

Member

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.

Member

acdlite commented Sep 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 22, 2017

Member

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

Doesn鈥檛 it mean we鈥檒l create two fibers for every fragment? One for the wrapper and one for the fragment itself. Seems unfortunate.

Member

gaearon commented Sep 22, 2017

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

Doesn鈥檛 it mean we鈥檒l create two fibers for every fragment? One for the wrapper and one for the fragment itself. Seems unfortunate.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Sep 22, 2017

Member

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

Member

acdlite commented Sep 22, 2017

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

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Sep 22, 2017

Member

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

Member

acdlite commented Sep 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@NE-SmallTown

NE-SmallTown Sep 23, 2017

Contributor

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?

Contributor

NE-SmallTown commented Sep 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Sep 23, 2017

Member

Doesn鈥檛 it mean we鈥檒l create two fibers for every fragment

Thought this was the case but then I remembered we don鈥檛 create an extra Fragment fiber for an array returned from render. For the same reason we don鈥檛 create an extra Fragment fiber for the children of a host component. It just becomes that fiber鈥檚 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.

Member

acdlite commented Sep 23, 2017

Doesn鈥檛 it mean we鈥檒l create two fibers for every fragment

Thought this was the case but then I remembered we don鈥檛 create an extra Fragment fiber for an array returned from render. For the same reason we don鈥檛 create an extra Fragment fiber for the children of a host component. It just becomes that fiber鈥檚 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

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Sep 23, 2017

Member

Could still be worthwhile for other reasons though

Member

acdlite commented Sep 23, 2017

Could still be worthwhile for other reasons though

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 23, 2017

Member

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.

Member

gaearon commented Sep 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@clemmy

clemmy Sep 25, 2017

Contributor

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

Contributor

clemmy commented Sep 25, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@trueadm

trueadm Sep 27, 2017

Contributor

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

Contributor

trueadm commented Sep 27, 2017

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

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Sep 27, 2017

Member

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

Member

sophiebits commented Sep 27, 2017

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

@clemmy

This comment has been minimized.

Show comment
Hide comment
@clemmy

clemmy Sep 27, 2017

Contributor

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

Contributor

clemmy commented Sep 27, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 27, 2017

Member

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.

Member

gaearon commented Sep 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 27, 2017

Member

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.

Member

gaearon commented Sep 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@trueadm

trueadm Sep 27, 2017

Contributor

@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.
Contributor

trueadm commented Sep 27, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 27, 2017

Member

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鈥檛 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鈥檇 only need React.Fragment as a fallback for people who can鈥檛 use the syntax yet (e.g. due to waiting for tools). I don鈥檛 see why the export would be useful for the syntax itself.

Member

gaearon commented Sep 27, 2017

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鈥檛 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鈥檇 only need React.Fragment as a fallback for people who can鈥檛 use the syntax yet (e.g. due to waiting for tools). I don鈥檛 see why the export would be useful for the syntax itself.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Sep 27, 2017

Member

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

Member

aweary commented Sep 27, 2017

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

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Sep 27, 2017

Member

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.

Member

acdlite commented Sep 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Sep 27, 2017

Member

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.

Member

acdlite commented Sep 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 27, 2017

Member

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.

Member

gaearon commented Sep 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 27, 2017

Member

But yes, either that, or let鈥檚 put it behind a feature flag. I don鈥檛 want this to become a public API before we鈥檙e truly committed to it.

Member

gaearon commented Sep 27, 2017

But yes, either that, or let鈥檚 put it behind a feature flag. I don鈥檛 want this to become a public API before we鈥檙e truly committed to it.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Sep 27, 2017

Member

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.

Member

acdlite commented Sep 27, 2017

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.

@trueadm trueadm closed this Sep 27, 2017

@trueadm trueadm reopened this Sep 27, 2017

@trueadm

This comment has been minimized.

Show comment
Hide comment
@trueadm

trueadm Sep 27, 2017

Contributor

Opps sorry about that, my phone went a bit crazy and closed the PR. :/

Contributor

trueadm commented Sep 27, 2017

Opps sorry about that, my phone went a bit crazy and closed the PR. :/

Ethan-Arrowood added a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017

Add Fragment as a named export to React (facebook#10783)
* 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

@cherniavskii cherniavskii referenced this pull request Dec 23, 2017

Merged

[WIP] Migration from wrapper divs to Fragments #129

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

This comment has been minimized.

@gaearon

gaearon Feb 9, 2018

Member

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.

@gaearon

gaearon Feb 9, 2018

Member

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

This comment has been minimized.

Show comment
Hide comment
@xyzdata

xyzdata Mar 13, 2018

good to go!

xyzdata commented Mar 13, 2018

good to go!

@xyzdata

This comment has been minimized.

Show comment
Hide comment
@xyzdata

xyzdata Mar 13, 2018

image

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

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

  1. ...fragments, which don鈥檛 require keys for static items:

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

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

xyzdata commented Mar 13, 2018

image

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

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

  1. ...fragments, which don鈥檛 require keys for static items:

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

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

@xyzdata xyzdata referenced this pull request Mar 13, 2018

Open

JS & skin #308

@clemmy

This comment has been minimized.

Show comment
Hide comment
@clemmy

clemmy Mar 13, 2018

Contributor

@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
Contributor

clemmy commented Mar 13, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@xyzdata

xyzdata Mar 13, 2018

@clemmy Thanks for your time.

good explanation.

xyzdata commented Mar 13, 2018

@clemmy Thanks for your time.

good explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment