Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Fix issues with context and undefined children #317

Closed
wants to merge 1 commit into from
Closed

Fix issues with context and undefined children #317

wants to merge 1 commit into from

Conversation

bkniffler
Copy link

No description provided.

@apollo-cla
Copy link

@bkniffler: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

Copy link
Contributor

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Would it be possible to add some tests that highlight why the if (child) checks are needed? It would avoid reversions in the future..

@@ -48,6 +48,8 @@ export function walkTree(
// https://github.com/facebook/react/blob/master/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js#L66
if (Component.prototype && Component.prototype.isReactComponent) {
const instance = new Component(props, context);
instance.props = props;
instance.context = context;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need these? I thought new Component did that for us?

Copy link
Author

Choose a reason for hiding this comment

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

These three constructors are valid for react:

constructor() {
super();
}

constructor(props) {
super(props);
}

constructor(props, context) {
super(props, context);
}

But only the last one is covered by new Component(props, context). At least thats what I suppose.

@bkniffler
Copy link
Author

I can't do it immediately, got some work to catch up. But there is a lot of situations where a child can be null, a stupid example: <MyComponent>{user.isAdmin ? <AdminPanel /> : null}{user.isSuperAdmin ? <SuperAdminPanel /> : null}</MyComponent>.

tmeasday added a commit that referenced this pull request Nov 10, 2016
@tmeasday
Copy link
Contributor

Thanks @bkniffler , see 800ce40

@tmeasday tmeasday closed this Nov 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants