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

Verify that the component passed to createAnimatedComponent is not functional #15019

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Jul 14, 2017

Stateless functional components don't support refs and we need that for the component to work, it used to crash with this error message: undefined is not an object (evaluating 'this._component.getScrollableNode'). This makes it clear what the issue is.

Fixes some of the errors in #10635, not sure if it fixes all the cases described in the issue though.

Test plan
Tested that passing a component with createClass or extends Component works but passing a function causes an error.

Release Notes

[GENERAL] [ENHANCEMENT] [Animated] - Verify that the component passed to createAnimatedComponent is not functional

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Jul 14, 2017
@@ -1732,6 +1732,12 @@ class AnimatedProps extends Animated {
}

function createAnimatedComponent(Component: any): any {
invariant(
Component.prototype.render,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better way to check this? It does work but meh...

@janicduplessis
Copy link
Contributor Author

@sahrens Could you have a look at this?

@pull-bot
Copy link

pull-bot commented Sep 9, 2017

Warnings
⚠️

📋 Release Notes - This PR appears to be missing Release Notes.

@facebook-github-bot label Needs more information

@facebook-github-bot label Core Team

Generated by 🚫 dangerJS

Copy link
Contributor

@sahrens sahrens left a comment

Choose a reason for hiding this comment

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

Hmm, is this the best way to check? I thought React had some helper functions that would be more robust.

@@ -17,7 +17,15 @@ const AnimatedProps = require('./nodes/AnimatedProps');
const React = require('React');
const ViewStylePropTypes = require('ViewStylePropTypes');

const invariant = require('fbjs/lib/invariant');

function createAnimatedComponent(Component: any): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

For Flow checking within the RN codebase, you could add something like this:

createAnimatedComponent<P>(
  Component: Class<React.Component<P>>
): Class<React.Component<*>> {
  ...
}

I believe this will filter out functions too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember trying to type this properly but I hit some issue. React components typing changed in flow 0.53 I can try again to see if I can figure out something that works.

@janicduplessis
Copy link
Contributor Author

@sahrens Yea I’m not sure if there is a better way, I commented on the diff to ask that but it doesn’t show up since I rebased. Maybe ask someone from the react team.

@janicduplessis
Copy link
Contributor Author

@bvaughn Is there a public API to check if a component is functional? (needed to know if refs are available).

@bvaughn
Copy link
Contributor

bvaughn commented Oct 3, 2017

@janicduplessis

FWIW we use this type of duck typing in React too (eg ReactFiberBeginWork, ReactTestUtilsEntry).

ReactBaseClass (aka React.Component, React.PureComponent) also defines an isReactComponent prop that we use some places (like in ReactFiber).

@facebook-github-bot
Copy link
Contributor

@janicduplessis I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@janicduplessis
Copy link
Contributor Author

Thanks @bvaughn ! Update to use isReactComponent.

@ide I played with trying to flow type it properly again but there was an issue with default props, it didn't seem to pick it up properly anymore so it caused a bunch of errors where some props that should not be required were.

@janicduplessis
Copy link
Contributor Author

ping @sahrens @bvaughn :)

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

Whoops, sorry for not seeing your earlier update. This looks good to me.

function createAnimatedComponent(Component: any): any {
invariant(
Component.prototype && Component.prototype.isReactComponent,
'`createAnimatedComponent` does not support stateless functional components, ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit: "," should be a ";" I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll trust you on that one. I never know when ; should be used, I guess I wasn't listening at school that day :)

@stale
Copy link

stale bot commented Jan 4, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 4, 2018
@stale stale bot closed this Jan 11, 2018
@ide
Copy link
Contributor

ide commented Jan 11, 2018

@janicduplessis would you want to rebase this?

@ide ide reopened this Jan 11, 2018
@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 11, 2018
@janicduplessis
Copy link
Contributor Author

@ide Looks like tests need to be updated to pass this new check, will try to find some time to update this soon.

@janicduplessis
Copy link
Contributor Author

Updated and re-tested, we actually need to allow string components too since in prod views are just RCTView strings.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 14, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@sahrens is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Plo4ox pushed a commit to Plo4ox/react-native that referenced this pull request Feb 17, 2018
…nctional

Summary:
Stateless functional components don't support refs and we need that for the component to work, it used to crash with this error message: `undefined is not an object (evaluating 'this._component.getScrollableNode')`. This makes it clear what the issue is.

Fixes some of the errors in facebook#10635, not sure if it fixes all the cases described in the issue though.

**Test plan**
Tested that passing a component with createClass or extends Component works but passing a function causes an error.

[GENERAL] [ENHANCEMENT] [Animated] - Verify that the component passed to createAnimatedComponent is not functional
Closes facebook#15019

Differential Revision: D6988096

Pulled By: sahrens

fbshipit-source-id: ec0ffa763245e786f44b4a1d56c0738876c25782
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. JavaScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants