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

ForwardRefs supports propTypes #12911

Merged
merged 3 commits into from May 29, 2018

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented May 25, 2018

Follow up to discussion on #12644

  • objects supports propTypes (and defaultProps, which already happened to work).
  • Add tests for defaultProps and failed propTypes warnings.
  • Split several internal-only ForwardRef tests off to be run against the built bundle also.

@bvaughn
Copy link
Contributor Author

bvaughn commented May 25, 2018

CI failure seems to be due to some (hopefully transient) Yarn registry problem. Not related to this PR.

Edit Seems like NPM just moved from Fastly to Cloudflare, and since Yarn is also using Cloudflare, it triggers a security error when Yarn forwards to NPM's registry. As a result, CI is going to be down for a bit until Yarn's fixed.

@pull-bot
Copy link

pull-bot commented May 26, 2018

Details of bundled changes.

Comparing: f35d989...dfa3dc6

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +1.0% +0.4% 57.2 KB 57.77 KB 15.89 KB 15.95 KB UMD_DEV
react.development.js +1.2% +0.3% 47.84 KB 48.41 KB 13.55 KB 13.59 KB NODE_DEV
React-dev.js +1.3% +0.6% 48.05 KB 48.67 KB 13.21 KB 13.28 KB FB_WWW_DEV

Generated by 🚫 dangerJS

const componentClass = element.type;
if (typeof componentClass !== 'function') {
const type = element.type;
let name, propTypes;
Copy link

Choose a reason for hiding this comment

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

Shouldn't it be const propTypes = type.propTypes after the typeof type check, as that's the only value that it gets assigned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current approach avoids reading propTypes from a possible null or undefined value (although that shouldn't actually happen in practice). I don't think it's a problem as-is.

) {
const functionName =
element.type.render.displayName || element.type.render.name || '';
return functionName !== '' ? `ForwardRef(${functionName})` : 'ForwardRef';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this look like getComponentName duplication here? Can we unify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's similar. But getComponentName accepts a Fiber and this is a ReactElement.

We could refactor both to use a shared helper, but it didn't seem like the obvious correct thing to do in this case.

) {
// ForwardRef
const functionName = type.render.displayName || type.render.name || '';
name = functionName !== '' ? `ForwardRef(${functionName})` : 'ForwardRef';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, I feel like this is getting out of hand a little bit. We're going to forget updating some of these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getting out of hand a bit

😝

We often inline things rather than reuse them. I made a judgement call. I'm happy to change it if you think I should.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Write everything twice. This is three times. :-)
But I don't care strongly.

element.type.$$typeof === REACT_FORWARD_REF_TYPE
) {
const functionName =
element.type.render.displayName || element.type.render.name || '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's hoist reading element.type so we don't keep repeating its reads?

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.

Thanks for splitting out the bundle tests.

I think it would be nice to avoid some repetition since we already have the "get name" logic in a helper. But not critical.

@bvaughn bvaughn merged commit 83f76e4 into facebook:master May 29, 2018
@bvaughn bvaughn deleted the forwardRef-propTypes-support branch May 29, 2018 16:50
bors bot added a commit to mozilla/delivery-console that referenced this pull request Jun 13, 2018
193: Update dependency react to v16.4.1 r=magopian a=renovate[bot]

This Pull Request updates dependency [react](https://github.com/facebook/react) from `v16.4.0` to `v16.4.1`



<details>
<summary>Release Notes</summary>

### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1641-June-13-2018)
[Compare Source](facebook/react@v16.4.0...v16.4.1)
##### React

* You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@&#8203;bvaughn] in [#&#8203;12911](`facebook/react#12911))
##### React DOM

* Fix a crash when the input `type` changes from some other types to `text`. ([@&#8203;spirosikmd] in [#&#8203;12135](`facebook/react#12135))
* Fix a crash in IE11 when restoring focus to an SVG element. ([@&#8203;ThaddeusJiang] in [#&#8203;12996](`facebook/react#12996))
* Fix a range input not updating in some cases. ([@&#8203;Illu] in [#&#8203;12939](`facebook/react#12939))
* Fix input validation triggering unnecessarily in Firefox. ([@&#8203;nhunzaker] in [#&#8203;12925](`facebook/react#12925))
* Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@&#8203;nhunzaker] in [#&#8203;12976](`facebook/react#12976))
* Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@&#8203;philipp-spiess] in [#&#8203;12966](`facebook/react#12966))
##### React DOM Server

* Fix an incorrect value being provided by new context API. ([@&#8203;ericsoderberghp] in [#&#8203;12985](`facebook/react#12985), [@&#8203;gaearon] in [#&#8203;13019](`facebook/react#13019))
##### React Test Renderer

* Allow multiple root children in test renderer traversal API. ([@&#8203;gaearon] in [#&#8203;13017](`facebook/react#13017))
* Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@&#8203;fatfisz] in [#&#8203;13030](`facebook/react#13030))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
bors bot added a commit to mythmon/corsica-tree-status that referenced this pull request Jun 14, 2018
25: Update react monorepo to v16.4.1 r=renovate[bot] a=renovate[bot]

This Pull Request renovates the package group "react monorepo".


-   [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1`
-   [react](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1`

# Release Notes
<details>
<summary>facebook/react</summary>

### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1641-June-13-2018)
[Compare Source](facebook/react@v16.4.0...v16.4.1)
##### React

* You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@&#8203;bvaughn] in [#&#8203;12911](`facebook/react#12911))
##### React DOM

* Fix a crash when the input `type` changes from some other types to `text`. ([@&#8203;spirosikmd] in [#&#8203;12135](`facebook/react#12135))
* Fix a crash in IE11 when restoring focus to an SVG element. ([@&#8203;ThaddeusJiang] in [#&#8203;12996](`facebook/react#12996))
* Fix a range input not updating in some cases. ([@&#8203;Illu] in [#&#8203;12939](`facebook/react#12939))
* Fix input validation triggering unnecessarily in Firefox. ([@&#8203;nhunzaker] in [#&#8203;12925](`facebook/react#12925))
* Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@&#8203;nhunzaker] in [#&#8203;12976](`facebook/react#12976))
* Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@&#8203;philipp-spiess] in [#&#8203;12966](`facebook/react#12966))
##### React DOM Server

* Fix an incorrect value being provided by new context API. ([@&#8203;ericsoderberghp] in [#&#8203;12985](`facebook/react#12985), [@&#8203;gaearon] in [#&#8203;13019](`facebook/react#13019))
##### React Test Renderer

* Allow multiple root children in test renderer traversal API. ([@&#8203;gaearon] in [#&#8203;13017](`facebook/react#13017))
* Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@&#8203;fatfisz] in [#&#8203;13030](`facebook/react#13030))

---


</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
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

5 participants