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

fix exception when ES6 class with no propTypes defined before calling requireNativeComponent #1260

Closed
wants to merge 4 commits into from

Conversation

kejinlu
Copy link
Contributor

@kejinlu kejinlu commented May 13, 2015

No description provided.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 13, 2015
@ide
Copy link
Contributor

ide commented May 13, 2015

I have a question - what kind of native view does not have custom props? All the native views so far have special props to customize them.

@@ -24,7 +24,8 @@ function verifyPropTypes(
}
var nativeProps = viewConfig.nativeProps;
for (var prop in nativeProps) {
if (!component.propTypes[prop] &&
if (component.propTypes != undefined &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use === or !== instead of their looser counterparts whenever possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just check truthiness without comparing to undefined.

@vjeux
Copy link
Contributor

vjeux commented May 14, 2015

Can you provide more context in how you experienced this error?

@kejinlu
Copy link
Contributor Author

kejinlu commented May 14, 2015

React Native "Native UI Components (iOS)" Guide

call requireNativeComponent before propTypes ,Then will experience this error

// MapView.js
var React = require('react-native');
var { requireNativeComponent } = React;

class MapView extends React.Component {
  render() {
    return <RCTMap {...this.props} />;
  }
}

var RCTMap = requireNativeComponent('RCTMap', MapView);

MapView.propTypes = {
  /**
   * When this property is set to `true` and a valid camera is associated
   * with the map, the camera’s pitch angle is used to tilt the plane
   * of the map. When this property is set to `false`, the camera’s pitch
   * angle is ignored and the map is always displayed as if the user
   * is looking straight down onto it.
   */
  pitchEnabled: React.PropTypes.bool,
};

module.exports = MapView;

@ide
Copy link
Contributor

ide commented May 14, 2015

I think there's a bug in that code, since pitchEnabled will not get verified (docs fix at #1271). However it's also bad to get a confusing error about undefined not being an object, so I think we should change verifyPropTypes to assert that component.propTypes is defined and throw a helpful error message when it's missing.

@sahrens
Copy link
Contributor

sahrens commented May 14, 2015

@ide is right - maybe change this PR to throw a more useful error?

@kejinlu
Copy link
Contributor Author

kejinlu commented May 14, 2015

Should I close the request?

@ide
Copy link
Contributor

ide commented May 14, 2015

@kejinlu can you update this PR so that it asserts?

@kejinlu
Copy link
Contributor Author

kejinlu commented May 14, 2015

@ide @sahrens I found another problem ,the "component.displayName" in the verifyPropTypes code always be undefined. Should set component.displayName with some value manually in the previous context?Or just use "component.name"

@sahrens
Copy link
Contributor

sahrens commented May 15, 2015

"component.displayName" in the verifyPropTypes code always be undefined

Looks like for ES6 classes name works, but for React.createClass displayName works.

@spicyj : any idea why this is inconsistent? Is there a better mechanism than just checking both like

'' + (component.name || component.displayName) + ' has no propTypes defined`'

@sahrens
Copy link
Contributor

sahrens commented May 15, 2015

import

@sahrens
Copy link
Contributor

sahrens commented May 15, 2015

Can you figure out the right way to include the component name in the error messages and make it consistent in the function?

@sahrens
Copy link
Contributor

sahrens commented May 15, 2015

@facebook-github-bot import

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/966263836738882/int_phab to review.

@sophiebits
Copy link
Contributor

displayName || name is the right way to do it.

ES6 classes name works, but for React.createClass displayName works.
@a2 a2 closed this in de27f1d May 21, 2015
mganandraj pushed a commit to mganandraj/react-native that referenced this pull request Aug 5, 2022
…nance

Bring over some component governance improvements to 0.68-stable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants