Skip to content

Show better error message when accessing React APIs on React Native#8099

Closed
satya164 wants to merge 2 commits into
react:masterfrom
callstack-internal:wrong-react
Closed

Show better error message when accessing React APIs on React Native#8099
satya164 wants to merge 2 commits into
react:masterfrom
callstack-internal:wrong-react

Conversation

@satya164

@satya164 satya164 commented Jun 14, 2016

Copy link
Copy Markdown
Contributor

Motivation:

Lots of examples on the web still have the old way to import React APIs from React Native. Also when someone upgrades to latest version of React Native without reading the release notes can get confused. This PR adds getters for createClass and Component and throws an error with a better error message when they are accessed.

screenshot_20160614-125622

Test plan:

Trying to use ReactNative.createClass or ReactNative.Component will throw an error with this error message.

There's currently a bug in symbolicateStackTrace which actually crashes the app after showing the error due to the stack being null when updating the stack trace. But that's a separate issue which should be fixed separately. For now, to prevent the crash, we need to add the following before the return statement here - https://github.com/facebook/react-native/blob/master/Libraries/JavaScriptAppEngine/Initialization/symbolicateStackTrace.js#L29,

if (json.error) {
  throw new Error(json.error);
}

There's still the issue of stack being null, which prevents us from showing a proper stack trace.

Related #7722

cc @mkonicek

@ghost

ghost commented Jun 14, 2016

Copy link
Copy Markdown

By analyzing the blame information on this pull request, we identified @sebmarkbage and @nicklockwood to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jun 14, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

parameter key Missing annotation

@satya164

Copy link
Copy Markdown
Contributor Author

I'll fix the flow error soon.

Comment thread Libraries/react-native/react-native.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible to make these __DEV__ only? (preferably the whole definition, otherwise just the method contents).

@satya164 satya164 Jun 14, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@javache thanks. will make the method contents __DEV__ only. I don't think I can do that for the whole block.

@satya164

Copy link
Copy Markdown
Contributor Author

@javache updated :)

@javache

javache commented Jun 14, 2016

Copy link
Copy Markdown
Contributor

@facebook-github-bot shipit

@ghost ghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jun 14, 2016
@ghost

ghost commented Jun 14, 2016

Copy link
Copy Markdown

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in 803cb61 Jun 14, 2016
@satya164 satya164 deleted the wrong-react branch June 14, 2016 13:30
grabbou pushed a commit that referenced this pull request Jun 21, 2016
Summary:
**Motivation:**

Lots of examples on the web still have the old way to import React APIs from React Native. Also when someone upgrades to latest version of React Native without reading the release notes can get confused. This PR adds getters for  `createClass` and `Component` and throws an error with a better error message when they are accessed.

![screenshot_20160614-125622](https://cloud.githubusercontent.com/assets/1174278/16034600/47c70222-3230-11e6-9fe4-1a3493708829.png)

**Test plan:**

Trying to use `ReactNative.createClass` or `ReactNative.Component` will throw an error with this error message.

There's currently a bug in `symbolicateStackTrace` which actually crashes the app after showing the error due to the `stack` being null when updating the stack trace. But that's a separate issue which should be fixed separately. For now, to prevent the crash, we need to add the following before the return statement here - https://github.com/facebook/react-native/blob/master/Libraries/JavaScriptAppEn
Closes #8099

Differential Revision: D3430468

Pulled By: javache

fbshipit-source-id: c098e51e1f2c276d87eca6da3bd91a457d7840c5
This pull request was closed.
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. Import Started This pull request has been imported. This does not imply the PR has been approved.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants