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

Moar Warnings #3171

Merged
merged 3 commits into from
Feb 17, 2015
Merged

Moar Warnings #3171

merged 3 commits into from
Feb 17, 2015

Conversation

sebmarkbage
Copy link
Collaborator

Warn if accessing .type on a factory

This was an important convenience as an upgrade path but shouldn't be necessary if you're using best-practice of calling createFactory in the consuming component.

Warn if using Maps as children

We're not sure if this is the way we want to support this API. It creates two ways of doing things. It may conflict with a model that lazily flattens all children into iterable flat lists.

It is convenient to avoid needing to explicitly redefine the key of Maps. However, this use case isn't as common as having an iterable where the key is on the value, not the key.

I'm also not comfortable with the feature testing that we're doing. It seems potentially fragile.

We still support other iterables though.

cc @leebyron

Warn if getDOMNode or isMounted is accessed in render

This is an anti-pattern that can be easily avoided by putting the logic in componentDidMount and componentDidUpdate instead.

Otherwise creates a dependency on stale data inside render without enforcing a two-pass render.

This was an important convenience as an upgrade path but shouldn't be
necessary if you're using best-practice of calling createFactory in the
consuming component.
We're not sure if this is the way we want to support this API. It creates
two ways of doing things.

It is convenient to avoid needing to explicitly redefine the key of Maps.
However, this use case isn't as common as having an iterable where the
key is on the value, not the key.
This is an anti-pattern that can be easily avoided by putting the logic
in componentDidMount and componentDidUpdate instead.

It creates a dependency on stale data inside render without enforcing
a two-pass render.
@sebmarkbage sebmarkbage mentioned this pull request Feb 17, 2015
23 tasks
sebmarkbage added a commit that referenced this pull request Feb 17, 2015
@sebmarkbage sebmarkbage merged commit cf4bef8 into facebook:master Feb 17, 2015
@leebyron
Copy link
Contributor

I'm also not comfortable with the feature testing that we're doing. It seems potentially fragile.

The testing we are doing is pretty close to the spec, so it should be robust. The spec describes all uses of @@Iterable properties as having the same function object value as the default iteration function property (values for Array and Set, entries for Map)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants