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

Add Fragment as a named export to React #10783

Merged
merged 46 commits into from
Oct 31, 2017

Conversation

clemmy
Copy link
Contributor

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

Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
});

it('should render via native renderer', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have said so sooner! :'(

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, good to know! :)

@acdlite
Copy link
Collaborator

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
Copy link
Collaborator

gaearon commented Sep 22, 2017

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
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
Copy link
Collaborator

acdlite commented Sep 23, 2017

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
Copy link
Collaborator

acdlite commented Sep 23, 2017

Could still be worthwhile for other reasons though

@gaearon
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor

trueadm commented Sep 27, 2017

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

@sophiebits
Copy link
Collaborator

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

@clemmy
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
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
Copy link
Collaborator

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’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
Copy link
Contributor

aweary commented Sep 27, 2017

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

@acdlite
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

gaearon commented Sep 27, 2017

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
Copy link
Collaborator

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
Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017
* 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
function validateFragmentProps(fragment) {
currentlyValidatingElement = fragment;

for (const key of Object.keys(fragment.props)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

xyzdata commented Mar 13, 2018

good to go!

@xyzdata
Copy link

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’t 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>

@clemmy
Copy link
Contributor Author

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
Copy link

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.