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

[Standalone for NativeComponents] RCTSlider #23048

Closed
wants to merge 3 commits into from
Closed

[Standalone for NativeComponents] RCTSlider #23048

wants to merge 3 commits into from

Conversation

OdaDaisuke
Copy link

@OdaDaisuke OdaDaisuke commented Jan 17, 2019

Changelog:

[iOS] [Changed] - Deal with #22990. Move requireNativeComponent to a separate file.

Test Plan:

  • yarn test
  • yarn flow
  • yarn lint

@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 Jan 17, 2019
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues.


const requireNativeComponent = require('requireNativeComponent');

module.exports = requireNativeComponent('RCTSlider');

Choose a reason for hiding this comment

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

prettier/prettier: Insert

Copy link
Author

Choose a reason for hiding this comment

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

I got it.


const requireNativeComponent = require('requireNativeComponent');

module.exports = requireNativeComponent('RCTSlider');

Choose a reason for hiding this comment

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

eol-last: Newline required at end of file but not found.

Copy link
Author

Choose a reason for hiding this comment

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

I got it.


const requireNativeComponent = require('requireNativeComponent');

module.exports = requireNativeComponent('RCTSlider');
Copy link
Member

Choose a reason for hiding this comment

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

We want to have flow types for these components defined in these files too. Take a look at some of the other PRs as an example.

Copy link
Author

Choose a reason for hiding this comment

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

I see. I'll modify these components.

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues.
  • flow found some issues.


type RCTSliderType = Class<NativeComponent<NativeProps>>;

module.exports = ((requireNativeComponent('RCTSlider'): any): RCTSliderType;

Choose a reason for hiding this comment

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

Unexpected token ;

Copy link
Member

Choose a reason for hiding this comment

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

I think you are missing a closing paren here.

import type {ImageSource} from 'ImageSource';
import type {ViewStyleProp} from 'StyleSheet';
import type {ColorValue} from 'StyleSheetTypes';
import type {ViewProps} from 'ViewPropTypes';
import type {SyntheticEvent} from 'CoreEventTypes';

const RCTSlider = requireNativeComponent('RCTSlider');
const RCTSliderNativeComponent = require('RCTSliderNativeComponent');

Choose a reason for hiding this comment

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

Importing from an untyped module makes it any and is not safe! Did you mean to add // @flow to the top of RCTSliderNativeComponent? (untyped-import)

Copy link
Member

@elicwhite elicwhite left a comment

Choose a reason for hiding this comment

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

It looks like the analyze job is failing on your PR. Can you go through the errors on that and work on getting that to pass?

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.

@cpojer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@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 Jan 22, 2019
@react-native-bot
Copy link
Collaborator

@OdaDaisuke merged commit 7e82e45 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Jan 22, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Jan 22, 2019
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019
matt-oakes pushed a commit to matt-oakes/react-native that referenced this pull request Feb 7, 2019
Summary:
Changelog:

[iOS] [Changed] - Deal with facebook#22990. Move `requireNativeComponent` to a separate file.
Pull Request resolved: facebook#23048

Differential Revision: D13760373

Pulled By: cpojer

fbshipit-source-id: ff8cc9d468dc3bac55fc3d4156ad695dcdd10ab8
michalchudziak pushed a commit to michalchudziak/react-native-slider that referenced this pull request Feb 8, 2019
Summary:
Changelog:

[iOS] [Changed] - Deal with #22990. Move `requireNativeComponent` to a separate file.
Pull Request resolved: facebook/react-native#23048

Differential Revision: D13760373

Pulled By: cpojer

fbshipit-source-id: ff8cc9d468dc3bac55fc3d4156ad695dcdd10ab8
james-watkin added a commit to james-watkin/react-native-slider that referenced this pull request Dec 28, 2021
Summary:
Changelog:

[iOS] [Changed] - Deal with #22990. Move `requireNativeComponent` to a separate file.
Pull Request resolved: facebook/react-native#23048

Differential Revision: D13760373

Pulled By: cpojer

fbshipit-source-id: ff8cc9d468dc3bac55fc3d4156ad695dcdd10ab8
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: Slider Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants