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

[Fiber] Child Reconciler + New Coroutines Primitive #6859

Merged
merged 2 commits into from May 27, 2016

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented May 24, 2016

First commit is expanding the structure of child reconciliation. The steps are organized around phase rather than type of component. I found this helpful for now but I'm sure other structures make sense too.

New Coroutine Primitive. Alternative APIs are possible. The important part is getting the implementation details right to have all the pieces available in the reconciler.

var element = getElement(unitOfWork);
var fn = element.type;
var props = element.props;
var value = fn(props);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this check shouldConstruct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I see. value is an object only if fn is module-pattern-y (like Relay containers currently are).

@sophiebits
Copy link
Collaborator

I think this makes sense overall. My two main points of confusion (already noted inline):

  1. completeWork getting called twice on coroutines and the existence of hasPendingChanges
  2. whether yields always need to be direct children of the corresponding coroutine (seems like intermediate composites are allowed, but no host components nor other coroutines).

// If there is more work to do in this parent, do that next.
return unitOfWork.sibling;
} else if (unitOfWork.parent) {
// If there's no more work in this parent. Complete the parent.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is how completion works by walking up the parent.

I was, however, considering using an explicit stack object that contains the path to the parent currently executing since that can be recreated whenever you start back from the top. That saves us having to store the parent pointer on the fiber itself.

I figured we might need the parent point for other things like finding effects, or dispatching events, but we could also do that using something like "host parent" or "event parent" like we do in the stack reconciler.

@ghost
Copy link

ghost commented May 27, 2016

@sebmarkbage updated the pull request.

@ghost
Copy link

ghost commented May 27, 2016

@sebmarkbage updated the pull request.

@sebmarkbage sebmarkbage changed the title RFC: [Fiber] Child Reconciler + New Coroutines Primitive [Fiber] Child Reconciler + New Coroutines Primitive May 27, 2016
@sebmarkbage
Copy link
Collaborator Author

Implemented the feedback. I'll leave the yield hack in for now.

@sebmarkbage sebmarkbage merged commit 0f4a4df into facebook:master May 27, 2016
@ghost
Copy link

ghost commented May 27, 2016

@sebmarkbage updated the pull request.

const fiber = exports.createFiberFromElementType(element.type);
if (typeof element.type === 'object') {
// Hacky McHack
element = ReactElement(fiber.input, null, element.ref, null, null, null, element.props);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this case for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Never mind, I get it now. element.type comes in as object, so fiber is just element.type, and we apply the props to the continuation type.

@zpao zpao modified the milestones: 16.0, 15-next Jun 1, 2016
@@ -184,7 +184,8 @@ function validatePropTypes(element) {
var ReactElementValidator = {

createElement: function(type, props, children) {
var validType = typeof type === 'string' || typeof type === 'function';
var validType = typeof type === 'string' || typeof type === 'function' ||
(type !== null && typeof type === 'object');
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 guess this is a minor-version change as it allows objects as element types. Should we maybe do a backport of this and exclude this change? We might also be able to get away with not cherry-picking this at all…

Copy link
Collaborator

Choose a reason for hiding this comment

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

This only changes the warning behavior, but it would be nice if we could be a little more strict here. Forgetting to export anything from a CommonJS module lands you an empty object here.

Copy link
Member

Choose a reason for hiding this comment

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

We could add and Object.keys(type).length check.

Regardless we probably don't want this part in 15 since it will change the warning behavior (at least before if you had the case you talk about you know something is about to go wrong).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is going to be throwing later on if you tried to render this anywhere before. It is also going to be throwing through out version 15 so even if you try a later version and then downgrades, it'll still not warn. So I think this is pretty safe.

We can be more specific once we know a bit more about the data structures that will be allowed here. I suspect "module components" might end up here if we do those. As well as yields.

zpao pushed a commit to zpao/react that referenced this pull request Jun 8, 2016
[Fiber] Child Reconciler + New Coroutines Primitive
(cherry picked from commit 0f4a4df)
@zpao zpao mentioned this pull request Jun 8, 2016
7 tasks
zpao pushed a commit that referenced this pull request Jun 14, 2016
[Fiber] Child Reconciler + New Coroutines Primitive
(cherry picked from commit 0f4a4df)
@zpao zpao modified the milestones: 15-next, 15.2.0 Jun 14, 2016
zpao added a commit to zpao/react that referenced this pull request Jun 15, 2016
zpao added a commit to zpao/react that referenced this pull request Jun 15, 2016
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.

None yet

4 participants