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

support Call and Return components in React.Children calls #11422

Merged
merged 6 commits into from Nov 20, 2017

Conversation

@MatteoVH
Copy link
Contributor

@MatteoVH MatteoVH commented Nov 1, 2017

Solves #11416

@gaearon, I tried to read up a bit on Call and Return, but I'm (obviously seeing as it's an experimental API) inexperienced with them. Let me know if I'm testing their usage satisfactorily!

@@ -28,6 +28,12 @@ var REACT_ELEMENT_TYPE =
const REACT_PORTAL_TYPE =
(typeof Symbol === 'function' && Symbol.for && Symbol.for('react.portal')) ||
0xeaca;
const REACT_CALL_TYPE =

This comment has been minimized.

@gaearon

gaearon Nov 1, 2017
Member

Minor nit: let's reorder them before the portal (the numbers go in sequential order).

This comment has been minimized.

@MatteoVH

MatteoVH Nov 1, 2017
Author Contributor

Makes sense.

@@ -23,6 +23,20 @@ describe('ReactChildren', () => {
ReactTestUtils = require('react-dom/test-utils');
});

const checkReactChildrenFunctionality = (element, callback, context) => {
const parentInstance = <div>{element}</div>;

This comment has been minimized.

@gaearon

gaearon Nov 1, 2017
Member

Let's just copy and paste this part between tests. It's fine for tests to be verbose. That way they're easier to debug.

Matteo Vesprini-Heidrich added 2 commits Nov 1, 2017
Matteo Vesprini-Heidrich
@MatteoVH
Copy link
Contributor Author

@MatteoVH MatteoVH commented Nov 1, 2017

Ready for re-review!

@@ -128,6 +134,8 @@ function traverseAllChildrenImpl(
// The following is inlined from ReactElement. This means we can optimize
// some checks. React Fiber also inlines this logic for similar purposes.
(type === 'object' && children.$$typeof === REACT_ELEMENT_TYPE) ||

This comment has been minimized.

@gaearon

gaearon Nov 1, 2017
Member

Can we return this into a switch somehow?

0xeac8;
const REACT_RETURN_TYPE =
(typeof Symbol === 'function' && Symbol.for && Symbol.for('react.return')) ||
0xeac9;
const REACT_PORTAL_TYPE =
(typeof Symbol === 'function' && Symbol.for && Symbol.for('react.portal')) ||

This comment has been minimized.

@nhunzaker

nhunzaker Nov 2, 2017
Collaborator

Does it make sense to share the typeof Symbol === 'function' && Symbol.for here? in a supportsSymbolFor variable, or something?

This comment has been minimized.

@gaearon

gaearon Nov 2, 2017
Member

Doesn't matter right now. This code executes once. Although it probably makes sense to copy how we do it in ReactChildFiber.

@gaearon
Copy link
Member

@gaearon gaearon commented Nov 6, 2017

Friendly ping :-)

@MatteoVH
Copy link
Contributor Author

@MatteoVH MatteoVH commented Nov 8, 2017

Will have time tonight to re-factor!

@MatteoVH MatteoVH force-pushed the MatteoVH:react-children-call-return-support branch from ba323c9 to 7eb6bba Nov 10, 2017
@MatteoVH
Copy link
Contributor Author

@MatteoVH MatteoVH commented Nov 10, 2017

Sorry for the late response, I tried to move some of the if logic into a switch, and it just didn't seem to cleanly fit into that construct. I left one attempt, but I think it's more legible as it was before @gaearon.

@MatteoVH MatteoVH force-pushed the MatteoVH:react-children-call-return-support branch from 7eb6bba to 3d22e79 Nov 10, 2017
(type === 'object' && children.$$typeof === REACT_ELEMENT_TYPE) ||
(type === 'object' && children.$$typeof === REACT_PORTAL_TYPE)
) {
const invokeCallback = () => {

This comment has been minimized.

@gaearon

gaearon Nov 10, 2017
Member

This is a very hot path. We don't want to create closures here. It's fine to duplicate code instead if necessary.

Matteo Vesprini-Heidrich
@gaearon
Copy link
Member

@gaearon gaearon commented Nov 20, 2017

Instead of duplicating, can we just set a boolean flag? It starts out being false. Under conditions, we flip it to true. Then we check it at the bottom, and fire the callback if it is true.

@gaearon gaearon merged commit c6bde7b into facebook:master Nov 20, 2017
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
@gaearon
Copy link
Member

@gaearon gaearon commented Nov 20, 2017

Nice. Thanks.

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

* support Call and Return components in React.Children calls

* make tests more verbose

* fix ordering of React component types

* cleanup conditional detection of children type

* directly inline callback invocation

* reduce callback invocation code re-use
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.