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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
284efed
Add Fragment as a named export to React
Sep 22, 2017
c26914b
Remove extra tests for Fragment
Sep 23, 2017
adb7c61
Change React.Fragment export to be a string '#fragment'
Oct 11, 2017
3804288
Fix fragment special case to work with 1 child
Oct 11, 2017
f201879
Add single child test for fragment export
Oct 11, 2017
3a21716
Move fragment definition to ReactEntry.js and render components for k…
Oct 12, 2017
480e8d1
Inline createFiberFromElementType into createFiberFromElement
Oct 12, 2017
29e6472
Update reconciliation to special case fragments
Oct 16, 2017
e62c4b4
Use same semantics as implicit childsets for ReactFragment
Oct 17, 2017
e0c0a1c
Add more fragment state preservation tests
Oct 17, 2017
c74af40
Export symbol instead of string for fragments
Oct 17, 2017
1f9ef58
Fix rebase breakages
Oct 17, 2017
14d3a1b
Re-apply prettier at 1.2.2
Oct 17, 2017
6df523d
Merge branches in updateElement
Oct 17, 2017
b4f17f6
Remove unnecessary check
Oct 17, 2017
6b374f8
Re-use createFiberFromFragment for fragment case
Oct 17, 2017
7ceb631
Simplyify branches by adding type field to fragment fiber
Oct 18, 2017
53969d3
Move branching logic for fragments to broader methods when possible.
Oct 18, 2017
8ecb60c
Add more tests for fragments
Oct 18, 2017
ab1a58e
Address Dan's feedback
Oct 18, 2017
b7fff43
Move REACT_FRAGMENT_TYPE into __DEV__ block for DCE
Oct 19, 2017
a6aca28
Change hex representation of REACT_FRAGMENT_TYPE to follow convention
Oct 19, 2017
372a62a
Remove unnecessary branching and isArray checks
Oct 19, 2017
8405465
Update test for preserving children state when keys are same
Oct 20, 2017
cee245c
Fix updateSlot bug and add more tests
Oct 20, 2017
b3bac19
Make fragment tests more robust by using ops pattern
Oct 20, 2017
198ad8c
Update jsx element validator to allow numbers and symbols
Oct 20, 2017
1a47984
Remove type field from fragment fiber
Oct 20, 2017
5edee97
Fork reconcileChildFibers instead of recursing
Oct 20, 2017
1539f19
Merge branch 'master' into react-fragment-export
Oct 20, 2017
367c7e6
Merge branch 'master' into react-fragment-export
Oct 20, 2017
59f6828
Use ternary if condition
Oct 25, 2017
d29bd31
Revamp fragment test suite:
Oct 25, 2017
8c070e6
Check output of renderer in fragment tests to ensure no silly busines…
Oct 26, 2017
545d0b9
Finish implementation of fragment reconciliation with desired behavior
Oct 27, 2017
c8a0752
Add reverse render direction for fragment tests
Oct 27, 2017
fe2dd4d
Merge branch 'master' into react-fragment-export
Oct 27, 2017
48bcaa7
Remove unneeded fragment branch in updateElement
Oct 27, 2017
b3864af
Add more test cases for ReactFragment
Oct 27, 2017
17ac4a2
Handle childless fragment in reconciler
Oct 27, 2017
27312d0
Support fragment flattening in SSR
Oct 27, 2017
87ab859
Clean up ReactPartialRenderer
Oct 28, 2017
f9443d1
Warn when non-key and children props are passed to fragments
Oct 28, 2017
97d1bdd
Add non-null key check back to updateSlot's array's case
clemmy Oct 30, 2017
71252b9
Add test for positional reconciliation in fragments
Oct 30, 2017
8cbc93d
Add warning for refs in fragments with stack trace
Oct 30, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 50 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMServerIntegration-test.js
Expand Up @@ -403,6 +403,55 @@ describe('ReactDOMServerIntegration', () => {
expect(parent.childNodes[2].tagName).toBe('P');
});

itRenders('a fragment with one child', async render => {
let e = await render(<React.Fragment><div>text1</div></React.Fragment>);
let parent = e.parentNode;
expect(parent.childNodes[0].tagName).toBe('DIV');
});

itRenders('a fragment with several children', async render => {
let Header = props => {
return <p>header</p>;
};
let Footer = props => {
return <React.Fragment><h2>footer</h2><h3>about</h3></React.Fragment>;
};
let e = await render(
<React.Fragment>
<div>text1</div>
<span>text2</span>
<Header />
<Footer />
</React.Fragment>,
);
let parent = e.parentNode;
expect(parent.childNodes[0].tagName).toBe('DIV');
expect(parent.childNodes[1].tagName).toBe('SPAN');
expect(parent.childNodes[2].tagName).toBe('P');
expect(parent.childNodes[3].tagName).toBe('H2');
expect(parent.childNodes[4].tagName).toBe('H3');
});

itRenders('a nested fragment', async render => {
let e = await render(
<React.Fragment>
<React.Fragment>
<div>text1</div>
</React.Fragment>
<span>text2</span>
<React.Fragment>
<React.Fragment>
<React.Fragment>{null}<p /></React.Fragment>{false}
</React.Fragment>
</React.Fragment>
</React.Fragment>,
);
let parent = e.parentNode;
expect(parent.childNodes[0].tagName).toBe('DIV');
expect(parent.childNodes[1].tagName).toBe('SPAN');
expect(parent.childNodes[2].tagName).toBe('P');
});

itRenders('an iterable', async render => {
const threeDivIterable = {
'@@iterator': function() {
Expand Down Expand Up @@ -435,6 +484,7 @@ describe('ReactDOMServerIntegration', () => {
// but server returns empty HTML. So we compare parent text.
expect((await render(<div>{''}</div>)).textContent).toBe('');

expect(await render(<React.Fragment />)).toBe(null);
expect(await render([])).toBe(null);
expect(await render(false)).toBe(null);
expect(await render(true)).toBe(null);
Expand Down
86 changes: 59 additions & 27 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Expand Up @@ -31,6 +31,12 @@ var escapeTextContentForBrowser = require('../shared/escapeTextContentForBrowser
var isCustomComponent = require('../shared/isCustomComponent');
var omittedCloseTags = require('../shared/omittedCloseTags');

var REACT_FRAGMENT_TYPE =
(typeof Symbol === 'function' &&
Symbol.for &&
Symbol.for('react.fragment')) ||
0xeacb;

// Based on reading the React.Children implementation. TODO: type this somewhere?
type ReactNode = string | number | ReactElement;
type FlatReactChildren = Array<null | ReactNode>;
Expand Down Expand Up @@ -206,6 +212,22 @@ function getNonChildrenInnerMarkup(props) {
return null;
}

function flattenTopLevelChildren(children: mixed): FlatReactChildren {
if (!React.isValidElement(children)) {
return toArray(children);
}
const element = ((children: any): ReactElement);
if (element.type !== REACT_FRAGMENT_TYPE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just an optimization right? Not semantic meaning. I wonder if this is just slowing down the normal case where this is true since the check will happen again then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to do less checks? I think that every way we re-write this, there still ends up being 3 checks. https://github.com/facebook/react/pull/10783/files/48bcaa7198ef5d97bfc78badd1636a836a076cda..27312d0ebbc3ebbdaa8dc48e6f70119847267389#r147533483

return [element];
}
const fragmentChildren = element.props.children;
if (!React.isValidElement(fragmentChildren)) {
return toArray(fragmentChildren);
}
const fragmentChildElement = ((fragmentChildren: any): ReactElement);
return [fragmentChildElement];
}

function flattenOptionChildren(children: mixed): string {
var content = '';
// Flatten children and warn if they aren't strings or numbers;
Expand Down Expand Up @@ -482,14 +504,8 @@ class ReactDOMServerRenderer {
makeStaticMarkup: boolean;

constructor(children: mixed, makeStaticMarkup: boolean) {
var flatChildren;
if (React.isValidElement(children)) {
// Safe because we just checked it's an element.
var element = ((children: any): ReactElement);
flatChildren = [element];
} else {
flatChildren = toArray(children);
}
const flatChildren = flattenTopLevelChildren(children);

var topFrame: Frame = {
// Assume all trees start in the HTML namespace (not totally true, but
// this is what we did historically)
Expand Down Expand Up @@ -569,26 +585,42 @@ class ReactDOMServerRenderer {
({child: nextChild, context} = resolve(child, context));
if (nextChild === null || nextChild === false) {
return '';
} else {
if (React.isValidElement(nextChild)) {
// Safe because we just checked it's an element.
var nextElement = ((nextChild: any): ReactElement);
return this.renderDOM(nextElement, context, parentNamespace);
} else {
var nextChildren = toArray(nextChild);
var frame: Frame = {
domNamespace: parentNamespace,
children: nextChildren,
childIndex: 0,
context: context,
footer: '',
};
if (__DEV__) {
((frame: any): FrameDev).debugElementStack = [];
}
this.stack.push(frame);
return '';
} else if (!React.isValidElement(nextChild)) {
const nextChildren = toArray(nextChild);
const frame: Frame = {
domNamespace: parentNamespace,
children: nextChildren,
childIndex: 0,
context: context,
footer: '',
};
if (__DEV__) {
((frame: any): FrameDev).debugElementStack = [];
}
this.stack.push(frame);
return '';
} else if (
((nextChild: any): ReactElement).type === REACT_FRAGMENT_TYPE
) {
const nextChildren = toArray(
((nextChild: any): ReactElement).props.children,
);
const frame: Frame = {
domNamespace: parentNamespace,
children: nextChildren,
childIndex: 0,
context: context,
footer: '',
};
if (__DEV__) {
((frame: any): FrameDev).debugElementStack = [];
}
this.stack.push(frame);
return '';
} else {
// Safe because we just checked it's an element.
var nextElement = ((nextChild: any): ReactElement);
return this.renderDOM(nextElement, context, parentNamespace);
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion packages/react-noop-renderer/src/ReactNoop.js
Expand Up @@ -512,7 +512,8 @@ var ReactNoop = {
log(
' '.repeat(depth) +
'- ' +
(fiber.type ? fiber.type.name || fiber.type : '[root]'),
// need to explicitly coerce Symbol to a string
(fiber.type ? fiber.type.name || fiber.type.toString() : '[root]'),
'[' + fiber.expirationTime + (fiber.pendingProps ? '*' : '') + ']',
);
if (fiber.updateQueue) {
Expand Down