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.

Sign up for GitHub

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

Jump to bottom

Expose additional private modules #3308

Closed
wants to merge 3 commits into from
Closed

Expose additional private modules #3308

wants to merge 3 commits into from

Conversation

brentvatne
Copy link
Collaborator

  • TextInputState as TextInput.State
  • Touchable
  • flattenStyle as StyleSheet.flatten
  • ReactNativeART as ART

Original discussion in #1821

@facebook-github-bot facebook-github-bot 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 Oct 9, 2015
@brentvatne brentvatne mentioned this pull request Oct 9, 2015
Closed
@ide ide assigned vjeux Oct 9, 2015
@vjeux
Copy link
Contributor

vjeux commented Oct 9, 2015

@facebook-github-bot shipit

@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/182740555392881/int_phab to review.

vjeux
vjeux reviewed Oct 9, 2015
View reviewed changes
website/server/extractDocs.js
@@ -225,6 +225,7 @@ var apis = [
'../Libraries/Storage/AsyncStorage.ios.js',
'../Libraries/Utilities/BackAndroid.android.js',
'../Libraries/CameraRoll/CameraRoll.js',
'../Libraries/Utilities/Dimensions.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

mind doing a different pull request for this file, it only lives in open source

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure thing

@brentvatne
Copy link
Collaborator Author

@vjeux - amended this PR to exclude the website change

@vjeux
Copy link
Contributor

vjeux commented Oct 9, 2015

@facebook-github-bot shipit

@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/182740555392881/int_phab to review.

@vjeux
Copy link
Contributor

vjeux commented Oct 9, 2015

There's a flow error

1 Flow error detected in root "Libraries/FBReactKit/js"!
=====Flow error #0====
Libraries/FBReactKit/js/react-native-github/Libraries/StyleSheet/StyleSheet.js line 74 col 1-18: assignment of property `flatten`
Libraries/FBReactKit/js/react-native-github/Libraries/StyleSheet/StyleSheet.js line 74 col 12-18: property `flatten`
Libraries/FBReactKit/js/react-native-github/Libraries/StyleSheet/StyleSheet.js line 62 col 7-16: statics of StyleSheet

@brentvatne
Copy link
Collaborator Author

@vjeux - fixed, although I'm not entirely sure what the type definition should look like here on flatten -- should I just copy and paste it over from flattenStyle? Seems a bit redundant

ide
ide reviewed Oct 10, 2015
View reviewed changes
Libraries/StyleSheet/StyleSheet.js
@@ -59,6 +60,8 @@ var StyleSheetValidation = require('StyleSheetValidation');
* subsequent uses are going to refer an id (not implemented yet).
*/
class StyleSheet {
static flatten:Function;
Copy link
Contributor

Choose a reason for hiding this comment

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

space after colon likethe other type decls? also if you wanted to take this further you could maybe write: static flatten: typeof flattenStyle http://flowtype.org/docs/typeof.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, space after : please

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah that's fantastic thanks @ide :)

@vjeux
Copy link
Contributor

vjeux commented Oct 10, 2015

Have you tested art from npm, does it work?

@brentvatne
Copy link
Collaborator Author

@vjeux - it should work -- before adding art to dependencies we could not require ReactNativeART in react-native.js because it would error out due to missing art module. I'll test installing from this local version tomorrow.

@brentvatne
Copy link
Collaborator Author

@vjeux - yup, it works. One thing I wasn't sure about is whether we should have the react-native init project link libART.a automatically -- I left it off for now but could go either way on it. cc @ide

@ide
Copy link
Contributor

ide commented Oct 11, 2015

I could go either way. I lean towards keeping it separate because art is a separate npm package.

@brentvatne
Copy link
Collaborator Author

@ide - in this diff I've included art as a dependency for React Native because otherwise we can't add ReactNativeART to the public interface (it can't be required)

@ide
Copy link
Contributor

ide commented Oct 11, 2015

ok. We need the packager to perform better static analysis of imports and cull dead code.

@vjeux
Copy link
Contributor

vjeux commented Oct 12, 2015

@facebook-github-bot shipit

@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/182740555392881/int_phab to review.

@javache
Copy link
Member

javache commented Oct 19, 2015

@brentvatne Could you rebase this change? Right now it won't merge cleanly.

@facebook-github-bot
Copy link
Contributor

@brentvatne updated the pull request.

@brentvatne
Copy link
Collaborator Author

@javache - rebased on top of master, should be good to go

@javache
Copy link
Member

javache commented Oct 25, 2015

@facebook-github-bot shipit

@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/182740555392881/int_phab to review.

sebmarkbage
sebmarkbage reviewed Oct 26, 2015
View reviewed changes
Libraries/StyleSheet/StyleSheet.js
@@ -69,4 +72,7 @@ class StyleSheet {
}
}

/* TODO(brentvatne) docs are needed for this */
StyleSheet.flatten = flattenStyle;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to move this logic to React and decouple this from styles. The refactoring I've done has decoupled this logic from the style prop.

This is similar to the React.Children.toArray helper so something like that on React might make sense.

It doesn't really hurt to have it here too for now but I don't think it ultimately belongs here.

@facebook-github-bot
Copy link
Contributor

@brentvatne updated the pull request.

@brentvatne brentvatne closed this in 0da2004 Nov 5, 2015
MattFoley pushed a commit to skillz/react-native that referenced this pull request Nov 9, 2015
@brentvatne
Expose additional private modules
06dde10 
Summary: - TextInputState as TextInput.State
- Touchable
- flattenStyle as StyleSheet.flatten
- ReactNativeART as ART

Original discussion in facebook#1821
Closes facebook#3308

Reviewed By: sebmarkbage

Differential Revision: D2527152

Pulled By: javache

fb-gh-sync-id: 19d4ef9d4c0e6587b9f0793e1ca624aebb034f3b
Crash-- pushed a commit to Crash--/react-native that referenced this pull request Dec 24, 2015
@brentvatne @Crash--
Expose additional private modules
efae049 
Summary: - TextInputState as TextInput.State
- Touchable
- flattenStyle as StyleSheet.flatten
- ReactNativeART as ART

Original discussion in facebook#1821
Closes facebook#3308

Reviewed By: sebmarkbage

Differential Revision: D2527152

Pulled By: javache

fb-gh-sync-id: 19d4ef9d4c0e6587b9f0793e1ca624aebb034f3b
@mkonicek mkonicek mentioned this pull request May 25, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@vjeux vjeux

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@brentvatne @vjeux @facebook-github-bot @ide @javache @sebmarkbage