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

Deduplicate "unknown prop" warning to ease pain of version updates #9488

Conversation

flarnie
Copy link
Contributor

@flarnie flarnie commented Apr 22, 2017

Note: There are other PRs open to fix some failing tests and I expect to rebase on top of those and fix any remaining failures before landing. Hope folks can review this anyway in light of this.

Some widely used libraries which depend on React have used syntax that
ends up passing non-standard props to default React components like
'dom' and 'div'. For example:

  // where this.props contains attributes like 'foo'
  return <div {...this.props} />;

Since we started throwing warnings for this syntax in Reactv15.2 some
libraries have not upgraded past v15.2.

Now, because of deprecations in 15.5, some libraries will only work with
Reactv15.5, and this causes a conflict when other libraries are pinned
at v15.1. The overall effect is that either there are version conflicts
or the "unknown prop" warning is thrown all over the place, and users of
these libraries can't fix the warnings.

See #9466 for more context.

We are deduping this warning in hopes that it allows more library
authors to update to the latest version of React. React v16.0 may take a
different approach to this issue. For some related discussion, see
#9477

Test Plan:

Before submitting a pull request, please make sure the following is done:

  1. Fork the repository and create your branch from master.
  2. If you've added code that should be tested, add tests!
  3. If you've changed APIs, update the documentation.
  4. Ensure the test suite passes (npm test).
  5. Make sure your code lints (npm run lint).
  6. Format your code with prettier (npm run prettier).
  7. Run the Flow typechecks (npm run flow).
  8. If you added or removed any tests, run ./scripts/fiber/record-tests before submitting the pull request, and commit the resulting changes.
  9. If you haven't already, complete the CLA.

Some widely used libraries which depend on React have used syntax that
ends up passing non-standard props to default React components like
'dom' and 'div'. For example:
```
  // where this.props contains attributes like 'foo'
  return <div {...this.props} />;
```

Since we started throwing warnings for this syntax in Reactv15.2 some
libraries have not upgraded past v15.2.

Now, because of deprecations in 15.5, some libraries will only work with
Reactv15.5, and this causes a conflict when other libraries are pinned
at v15.1. The overall effect is that either there are version conflicts
or the "unknown prop" warning is thrown all over the place, and users of
these libraries can't fix the warnings.

See facebook#9466 for more context.

We are deduping this warning in hopes that it allows more library
authors to update to the latest version of React. React v16.0 may take a
different approach to this issue. For some related discussion, see
facebook#9477

**Test Plan:**
 - Added a unit test
 - Manually tested with a fixture that was not committed; https://gist.github.com/flarnie/db011bf54206f30b9983cd4dc674c82e
@sophiebits
Copy link
Contributor

Does this mean that if a third-party component warns first, then you'll never see any warnings for first-party code?

@gaearon
Copy link
Collaborator

gaearon commented Apr 22, 2017

Yea. IMO at this point it’s a reasonable tradeoff (most third party libs have updated long ago).

@flarnie
Copy link
Contributor Author

flarnie commented Apr 22, 2017

Yea - there are definitely trade-offs with this approach. It sounds like there was some discussion on Twitter, and then on the issue as well. I think this is okayish for v15.6, but we may want to discuss more how to deal with this in v16.0.

@gaearon
Copy link
Collaborator

gaearon commented Apr 22, 2017

IMO we should completely switch to the new behavior in 16.0 (#9477). Then we won't need to come back to this warning at all.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@flarnie
Copy link
Contributor Author

flarnie commented Apr 22, 2017

Holding off merging this until the React core team discusses this approach more. @spicyj @gaearon

@gaearon
Copy link
Collaborator

gaearon commented May 8, 2017

Closing per discussion with @flarnie and #9466 (comment).

@gaearon gaearon closed this May 8, 2017
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.

4 participants