Skip to content

Conversation

@sebmarkbage
Copy link
Contributor

I made this mistake while upgrading a few callers.

I'm leaving this as warning so that it is always safe to wrap any existing
child in React.addons.createFragment without breaking prod.

This makes upgrading easier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These arguments look a little mixed up. Shouldn't the format string be the second arg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Probably merge issue. I had the conditional inline first and then needed to return early for this conditional so I broke it out.

I made this mistake while upgrading a few callers.

I'm leaving this as warning so that it is always safe to wrap any existing
child in React.addons.createFragment without breaking prod.

This makes upgrading easier.
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to be stricter and also warn for arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about iterables? Maps?

Copy link
Member

Choose a reason for hiding this comment

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

Meh, make it as strict as you want.

sebmarkbage added a commit that referenced this pull request Feb 11, 2015
Warn if a non-object value is used in ReactFragment.create
@sebmarkbage sebmarkbage merged commit 032fb6c into facebook:master Feb 11, 2015
@sebmarkbage
Copy link
Contributor Author

We might want to disallow Maps without a ReactFragment wrapper. Will revisit the constraints here after that.

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.

3 participants