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.

Sign up for GitHub

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

Support fragments in <Switch /> #5892

Closed
wants to merge 1 commit into from
Closed

Support fragments in <Switch /> #5892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
52 changes: 36 additions & 16 deletions packages/react-router/modules/Switch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,29 +42,49 @@ class Switch extends React.Component {
const { route } = this.context.router;
const { children } = this.props;
const location = this.props.location || route.location;
const { match, child } = this.getMatch(route, location, children);

return match
? React.cloneElement(child, { location, computedMatch: match })
: null;
}

getMatch(route, location, children) {
let child = null;
let match = null;

let match, child;
React.Children.forEach(children, element => {
if (match == null && React.isValidElement(element)) {
const {
path: pathProp,
exact,
strict,
sensitive,
from
} = element.props;
const path = pathProp || from;
if (
React.Fragment != null && // Fragment support is only available in React.js >= 16.2
element.type === React.Fragment
) {
const subMatch = this.getMatch(
route,
location,
element.props.children
);
child = subMatch.child;
match = subMatch.match;
} else {
const {
path: pathProp,
exact,
strict,
sensitive,
from
} = element.props;
const path = pathProp || from;

child = element;
match = path
? matchPath(location.pathname, { path, exact, strict, sensitive })
: route.match;
child = element;
match = path
? matchPath(location.pathname, { path, exact, strict, sensitive })
: route.match;
}
}
});

return match
? React.cloneElement(child, { location, computedMatch: match })
: null;
return { child, match };
}
}

Expand Down
118 changes: 118 additions & 0 deletions packages/react-router/modules/__tests__/Switch-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,124 @@ describe("A <Switch>", () => {
expect(node.innerHTML).toMatch(/one/);
});

it("does handle fragments", () => {
const node = document.createElement("div");

ReactDOM.render(
<MemoryRouter initialEntries={["/two"]}>
<Switch>
<Route path="/one" render={() => <h1>one</h1>} />
<React.Fragment>
<Route path="/two" render={() => <h1>two</h1>} />
</React.Fragment>
<Route path="/three" render={() => <h1>three</h1>} />
</Switch>
</MemoryRouter>,
node
);

expect(node.innerHTML).toMatch(/two/);
});

it("does handle nested fragments", () => {
Copy link
Member

Choose a reason for hiding this comment

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

We specifically should not support this. We don't support nested routes in a Switch, so this behavior difference will undoubtedly cause confusion. To be clear, I see this:

<Switch>
  <Route path="/foo" component={Foo} />
  <>
    <Route path="/bar" component={Bar} />
    <Route path="/baz" component={Baz} />
  </>
</Switch>

as equivalent to this:

<Switch>
  <Route path="/foo" component={Foo} />
  {[barRoute, bazRoute].map(route => <Route path={route.path} component={route.component} />)}
</Switch>

Fragments are basically convenience functions for arrays. So, I'd treat them as such to maintain the status quo with Switch's behaviors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually React.Children.forEach recurses over nested arrays. Meaning: react-router always supported this very same case, i.e. arrays in arrays. For clarification, I added more test cases that use arrays instead of fragments. The behavior is only consistent when recursing over fragments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See: https://github.com/ReactTraining/react-router/pull/5892/files#diff-00d924998d3d7fd7c087362a6e4bc6bcR363

Copy link
Contributor Author

@bripkens bripkens Jan 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Relevant React code:

https://github.com/facebook/react/blob/4ca7855ca062d5d7dfca83c86acf46731e1e57ef/packages/react/src/ReactChildren.js#L153

and docs:

https://reactjs.org/docs/react-api.html#reactchildrentoarray

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One might argue that this is an inconsistency within React itself. Related:

facebook/react#11859 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I'm not. Sorry, just busted out some code here and contradicted myself. Apologies for the flip flop!

React.Children.count([<div/>, <div/>, [<div />, <div/>]]) == 4
React.Children.count([
  <div/>,
  <div/>,
  <React.Fragment>
    <div/>
    <div/>
  </React.Fragment>
]) == 3

Fragments are not the same as arrays, they just have a similar use case. The post here is clarifying: facebook/react#11859 (comment)

I'd much rather do React.Children.map(children, child => child.type === React.Fragment ? child.props.children : child).forEach(child => ...). It's not common to have nested child arrays inside of <Switch>, so trying to emulate that behavior with something that's intentionally inconsistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not see why fragments should be handled differently for all intents and purposes of react-router than arrays. They are element containers and should be handled accordingly.

Design decisions have been made for fragments which result in different behavior for React.Children.count, React.Children.map and others when comparing fragment and array behavior. This difference in behavior stems from important differences when handling these containers as part of render. React-router doesn't have to behave different when encountering fragments vs. arrays.

Furthermore, it makes no sense for react-router to try to matchPath a fragment element. This will never work.

Lastly, there is no technical reason why fragment recursion cannot be supported. There is no additional cost associated to it. It is faster than a map followed by a forEach. It makes array/fragment behavior consistent.

Copy link
Member

Choose a reason for hiding this comment

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

I do not see why fragments should be handled differently for all intents and purposes of react-router than arrays.

Because they're not arrays. I'd really rather not try to go against the upstream behavior semantics, even if it is admittedly weird. We got burned in previous versions by trying to do too much of React's behavior on our own. The edge cases are death by a thousand cuts.

I'm still on Team No Flatten™, but the rules dictate we have two maintainers approve before merging. If anyone else wants to voice their opinions, even if it's different from my own, they can and we can work towards a resolution.

Copy link

Choose a reason for hiding this comment

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

I'm not a maintainer, but I recommended this change over in #5785 , so I thought I'd voice that I'm also on Team No Flatten for whatever that's worth. Thanks for all of your work @timdorr and @bripkens!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the requested example to the issue:

#5785 (comment)

const node = document.createElement("div");

ReactDOM.render(
<MemoryRouter initialEntries={["/three"]}>
<Switch>
<Route path="/one" render={() => <h1>one</h1>} />
<React.Fragment>
<Route path="/two" render={() => <h1>two</h1>} />
<React.Fragment>
{null}
<Route path="/three" render={() => <h1>three</h1>} />
</React.Fragment>
</React.Fragment>
<Route path="/four" render={() => <h1>four</h1>} />
</Switch>
</MemoryRouter>,
node
);

expect(node.innerHTML).toMatch(/three/);
});

it("does not stop on nested fragments", () => {
const node = document.createElement("div");

ReactDOM.render(
<MemoryRouter initialEntries={["/three"]}>
<Switch>
<Route path="/one" render={() => <h1>one</h1>} />
<React.Fragment>
<Route path="/two" render={() => <h1>two</h1>} />
</React.Fragment>
<Route path="/three" render={() => <h1>three</h1>} />
</Switch>
</MemoryRouter>,
node
);

expect(node.innerHTML).toMatch(/three/);
});

it("does handle arrays", () => {
const node = document.createElement("div");

ReactDOM.render(
<MemoryRouter initialEntries={["/two"]}>
<Switch>
<Route path="/one" render={() => <h1>one</h1>} />
{[<Route key={2} path="/two" render={() => <h1>two</h1>} />]}
<Route path="/three" render={() => <h1>three</h1>} />
</Switch>
</MemoryRouter>,
node
);

expect(node.innerHTML).toMatch(/two/);
});

it("does handle nested arrays", () => {
const node = document.createElement("div");

ReactDOM.render(
<MemoryRouter initialEntries={["/three"]}>
<Switch>
<Route path="/one" render={() => <h1>one</h1>} />
{[
<Route key={2} path="/two" render={() => <h1>two</h1>} />,
[
null,
<Route key={3} path="/three" render={() => <h1>three</h1>} />
]
]}
<Route path="/four" render={() => <h1>four</h1>} />
</Switch>
</MemoryRouter>,
node
);

expect(node.innerHTML).toMatch(/three/);
});

it("does not stop on nested arrays", () => {
const node = document.createElement("div");

ReactDOM.render(
<MemoryRouter initialEntries={["/three"]}>
<Switch>
<Route path="/one" render={() => <h1>one</h1>} />
{[<Route key={2} path="/two" render={() => <h1>two</h1>} />]}
<Route path="/three" render={() => <h1>three</h1>} />
</Switch>
</MemoryRouter>,
node
);

expect(node.innerHTML).toMatch(/three/);
});

it("throws with no <Router>", () => {
const node = document.createElement("div");

Expand Down