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 33 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/react-noop-renderer/src/ReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,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
227 changes: 205 additions & 22 deletions packages/react-reconciler/src/ReactChildFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,11 @@ const FAUX_ITERATOR_SYMBOL = '@@iterator'; // Before Symbol spec.
const REACT_ELEMENT_TYPE =
(typeof Symbol === 'function' && Symbol.for && Symbol.for('react.element')) ||
0xeac7;
const REACT_FRAGMENT_TYPE =
(typeof Symbol === 'function' &&
Symbol.for &&
Symbol.for('react.fragment')) ||
0xeacb;

function getIteratorFn(maybeIterable: ?any): ?() => ?Iterator<*> {
if (maybeIterable === null || typeof maybeIterable === 'undefined') {
Expand Down Expand Up @@ -375,27 +380,35 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
element: ReactElement,
expirationTime: ExpirationTime,
): Fiber {
if (current === null || current.type !== element.type) {
// Insert
const created = createFiberFromElement(
element,
returnFiber.internalContextTag,
expirationTime,
);
created.ref = coerceRef(current, element);
created.return = returnFiber;
return created;
} else {
// TODO: Split these into branches based on typeof type
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? Can you point me to discussion that explains this TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#11290 It's to ensure correctness between type checking when comparing across functions, classes, strings, and potentially numbers/symbols.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should split based on tag, not typeof. It doesn't really matter if you're going to check the type anyway. Some of the other duplicates of this comment is confusing since in those cases you already do that branching.

if (
current !== null &&
(current.tag === Fragment
? element.type === REACT_FRAGMENT_TYPE
: current.type === element.type)
) {
// Move based on index
const existing = useFiber(current, expirationTime);
existing.ref = coerceRef(current, element);
existing.pendingProps = element.props;
existing.pendingProps = element.type === REACT_FRAGMENT_TYPE
? element.props.children
: element.props;
existing.return = returnFiber;
if (__DEV__) {
existing._debugSource = element._source;
existing._debugOwner = element._owner;
}
return existing;
} else {
// Insert
const created = createFiberFromElement(
element,
returnFiber.internalContextTag,
expirationTime,
);
created.ref = coerceRef(current, element);
created.return = returnFiber;
return created;
}
}

Expand Down Expand Up @@ -483,13 +496,15 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
current: Fiber | null,
fragment: Iterable<*>,
expirationTime: ExpirationTime,
key: null | string,
): Fiber {
if (current === null || current.tag !== Fragment) {
// Insert
const created = createFiberFromFragment(
fragment,
returnFiber.internalContextTag,
expirationTime,
key,
);
created.return = returnFiber;
return created;
Expand Down Expand Up @@ -570,6 +585,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
newChild,
returnFiber.internalContextTag,
expirationTime,
null,
);
created.return = returnFiber;
return created;
Expand Down Expand Up @@ -616,6 +632,15 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
switch (newChild.$$typeof) {
case REACT_ELEMENT_TYPE: {
if (newChild.key === key) {
if (newChild.type === REACT_FRAGMENT_TYPE) {
return updateFragment(
returnFiber,
oldFiber,
newChild.props.children,
expirationTime,
key,
);
}
return updateElement(
returnFiber,
oldFiber,
Expand Down Expand Up @@ -666,12 +691,13 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
}

if (isArray(newChild) || getIteratorFn(newChild)) {
// Fragments don't have keys so if the previous key is implicit we can
// update it.
if (key !== null) {
return null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

updateFragment assumes that key equality has already been checked. I suspect that removing this check means that if there was a previous key (e.g. because it was a keyed fragment) it'll try to reuse that Fiber. We should probably have a test for that.

<div>{<Fragment key="foo"><Stateful /></Fragment>}<span /></div>
->
<div>{[<Stateful />]}<span /></div>

return updateFragment(returnFiber, oldFiber, newChild, expirationTime);
return updateFragment(
returnFiber,
oldFiber,
newChild,
expirationTime,
key,
);
}

throwOnInvalidObjectType(returnFiber, newChild);
Expand Down Expand Up @@ -712,6 +738,15 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
existingChildren.get(
newChild.key === null ? newIdx : newChild.key,
) || null;
if (newChild.type === REACT_FRAGMENT_TYPE) {
return updateFragment(
returnFiber,
matchedFiber,
newChild.props.children,
expirationTime,
newChild.key,
);
}
return updateElement(
returnFiber,
matchedFiber,
Expand Down Expand Up @@ -766,6 +801,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
matchedFiber,
newChild,
expirationTime,
null,
);
}

Expand Down Expand Up @@ -1203,11 +1239,18 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
// TODO: If key === null and child.key === null, then this only applies to
// the first item in the list.
if (child.key === key) {
if (child.type === element.type) {
// TODO: Split these into branches based on typeof type
if (
child.tag === Fragment
? element.type === REACT_FRAGMENT_TYPE
: child.type === element.type
) {
deleteRemainingChildren(returnFiber, child.sibling);
const existing = useFiber(child, expirationTime);
existing.ref = coerceRef(child, element);
existing.pendingProps = element.props;
existing.pendingProps = element.type === REACT_FRAGMENT_TYPE
? element.props.children
: element.props;
existing.return = returnFiber;
if (__DEV__) {
existing._debugSource = element._source;
Expand Down Expand Up @@ -1342,9 +1385,140 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
return created;
}

// A fork of reconcileChildFibers to be used when reconcileChildFibers
// encounters a top level fragment to treat it as a set of children.
// This function is not recursive.
function reconcileChildFibersForTopLevelFragment(
returnFiber: Fiber,
currentFirstChild: Fiber | null,
newChild: any,
expirationTime: ExpirationTime,
): Fiber | null {
// This function is not recursive.
// If the top level item is an array, we treat it as a set of children,
// not as a fragment. Nested arrays on the other hand will be treated as
// fragment nodes. Recursion happens at the normal flow.

// Handle object types
const isObject = typeof newChild === 'object' && newChild !== null;
if (isObject) {
switch (newChild.$$typeof) {
case REACT_ELEMENT_TYPE:
return placeSingleChild(
reconcileSingleElement(
returnFiber,
currentFirstChild,
newChild,
expirationTime,
),
);
case REACT_COROUTINE_TYPE:
return placeSingleChild(
reconcileSingleCoroutine(
returnFiber,
currentFirstChild,
newChild,
expirationTime,
),
);
case REACT_YIELD_TYPE:
return placeSingleChild(
reconcileSingleYield(
returnFiber,
currentFirstChild,
newChild,
expirationTime,
),
);
case REACT_PORTAL_TYPE:
return placeSingleChild(
reconcileSinglePortal(
returnFiber,
currentFirstChild,
newChild,
expirationTime,
),
);
}
}

if (typeof newChild === 'string' || typeof newChild === 'number') {
return placeSingleChild(
reconcileSingleTextNode(
returnFiber,
currentFirstChild,
'' + newChild,
expirationTime,
),
);
}

if (isArray(newChild)) {
Copy link
Collaborator

@sebmarkbage sebmarkbage Oct 21, 2017

Choose a reason for hiding this comment

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

Doesn't this mean that return <>{[...]}</> is treated the same as return <>{...}</>. That's not the same as return [[...]] vs return [...]. It flattens an extra level because reconcileChildrenArray avoids the extra fragment but in this case there should always be a fragment.

We should probably have a test for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I gave you bad advice. The function you wanted to fork was updateSlot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bringing this up made me think about some other permutations of fragment and array combinations given our current desired behavior of top-level fragments behaving the same as a top-level array. I jotted some of them down here (with the ones I'm unsure about marked with ?:
https://gist.github.com/clemmy/b3ef00f9507909429d8aa0d3ee4f986b
Does that look right to you?

return reconcileChildrenArray(
returnFiber,
currentFirstChild,
newChild,
expirationTime,
);
}

if (getIteratorFn(newChild)) {
return reconcileChildrenIterator(
returnFiber,
currentFirstChild,
newChild,
expirationTime,
);
}

if (isObject) {
throwOnInvalidObjectType(returnFiber, newChild);
}

if (__DEV__) {
if (typeof newChild === 'function') {
warnOnFunctionType();
}
}
if (typeof newChild === 'undefined') {
// If the new child is undefined, and the return fiber is a composite
// component, throw an error. If Fiber return types are disabled,
// we already threw above.
switch (returnFiber.tag) {
case ClassComponent: {
if (__DEV__) {
const instance = returnFiber.stateNode;
if (instance.render._isMockFunction) {
// We allow auto-mocks to proceed as if they're returning null.
break;
}
}
}
// Intentionally fall through to the next case, which handles both
// functions and classes
// eslint-disable-next-lined no-fallthrough
case FunctionalComponent: {
const Component = returnFiber.type;
invariant(
false,
'%s(...): Nothing was returned from render. This usually means a ' +
'return statement is missing. Or, to render nothing, ' +
'return null.',
Component.displayName || Component.name || 'Component',
);
}
}
}

// Remaining cases are all treated as empty.
return deleteRemainingChildren(returnFiber, currentFirstChild);
}

// This API will tag the children with the side-effect of the reconciliation
// itself. They will be added to the side-effect list as we pass through the
// children and the parent.
// This function is forked at reconcileChildFibersForTopLevelFragment
// Please reflect changes in this function to its fork as well
function reconcileChildFibers(
returnFiber: Fiber,
currentFirstChild: Fiber | null,
Expand All @@ -1361,6 +1535,17 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
if (isObject) {
switch (newChild.$$typeof) {
case REACT_ELEMENT_TYPE:
// This function recurses only on a top-level fragment,
// so that it is treated as a set of children
// Otherwise, we follow the normal flow.
if (newChild.type === REACT_FRAGMENT_TYPE && newChild.key === null) {
return reconcileChildFibersForTopLevelFragment(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually think you don't even need to fork anything. You just need to call updateSlot here instead.

returnFiber,
currentFirstChild,
newChild.props.children,
expirationTime,
);
}
return placeSingleChild(
reconcileSingleElement(
returnFiber,
Expand All @@ -1369,7 +1554,6 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
expirationTime,
),
);

case REACT_COROUTINE_TYPE:
return placeSingleChild(
reconcileSingleCoroutine(
Expand All @@ -1388,7 +1572,6 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
expirationTime,
),
);

case REACT_PORTAL_TYPE:
return placeSingleChild(
reconcileSinglePortal(
Expand Down
Loading