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

[Public Interface] Create whitelist internal modules that need to be exposed publicly #1821

Closed
brentvatne opened this issue Jun 30, 2015 · 27 comments
Assignees
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@brentvatne
Copy link
Collaborator

As pointed out in #1808, on 0.7.0-rc+ requiring modules that are internal to React Native (eg: require('NativeModules') rather than var { NativeModules } = require('react-native')) is not supported going forward. The new behavior is correct, so we need to help people transition away from the old way. A good example of using this incorrect behavior to access internals can be seen on my react-native-video project.

The purpose of this issue is to ensure that we publicly expose every module that is necessary to build libraries and apps - to do this we simply have to make a whitelist of those modules and add them to react-native.js)

Please add to this issue with internal modules that you use on your projects that are not already listed here. Thanks!

cc @vjeux @amasad

@brentvatne brentvatne self-assigned this Jun 30, 2015
@brentvatne brentvatne changed the title [Packager] Prevent requiring internals of React [Public Interface] Create whitelist internal modules that need to be exposed publicly Jun 30, 2015
@ide
Copy link
Contributor

ide commented Jun 30, 2015

Dimensions is a big one. Many of the prop types are either exported or you can write View.propTypes, Text.propTypes, etc.

@orktes
Copy link

orktes commented Jul 1, 2015

precomputeStyle is one that I think people (me included) use frequently

@brentvatne
Copy link
Collaborator Author

For your consideration, @ide @ccheever @orktes:

Likely candidates for inclusion

  • ReactNativeART (ART)
  • Dimensions
  • Platform
  • flattenStyle (StyleSheet.flatten)
  • ViewStylePropTypes
  • TextStylePropTypes
  • LayoutPropTypes
  • StyleSheetPropType
  • ReactNativeViewAttributes
  • NativeMethodsMixin (look into this one more)
  • deepDiffer (and other differs)
  • precomputeStyle
  • NavigatorNavigationBarStyles
  • StyleSheetRegistry
  • StyleSheetValidation
  • TextInputState (TextInput.State)

Not sure what we should do with these

  • ReactInstanceMap (via react-tools) (look into this one further)
  • Touchable (via react-contrib, used in several projects)
  • TimerMixin (via react-timer-mixin)
  • onlyChild (via react-tools, already available via require('react-native').Children.only)
  • invariant (via react-tools)
  • rebound
  • StaticContainer (via react-contrib)
  • logError
  • warning (exposed globally, probably not necessary but require'd here: https://github.com/rt2zz/react-native-addressbook/blob/master/index.js)
  • merge
  • keyMirror

Documentation

  • Dimensions
  • Go through others to make sure that public APIs have this

(Mostly) complete list of repos that will be affected

https://github.com/maxs15/react-native-screcorder/blob/master/recorder.ios.js#L1-L11
https://github.com/oblador/react-native-vector-icons/blob/master/lib/create-icon-set.js
https://github.com/paramaggarwal/react-native-youtube/blob/master/YouTube.ios.js
https://github.com/brentvatne/react-native-video/blob/master/Video.ios.js
https://github.com/umhan35/react-native-search-bar/blob/master/SearchBar.js
https://github.com/kirkness/react-native-swift-socketio/blob/master/index.js
https://github.com/magus/react-native-facebook-login/blob/master/FBLogin.ios.js
https://github.com/chirag04/react-native-tooltip/blob/master/ToolTipText.ios.js
https://github.com/chirag04/react-native-tooltip/blob/master/ToolTipText.ios.js
https://github.com/brentvatne/react-native-scrollable-tab-view/blob/master/index.js
https://github.com/naoufal/react-native-safari-view/blob/master/SafariViewManager.ios.js
https://github.com/dsibiski/react-native-userdefaults-ios/blob/master/RNUserDefaultsIOS.js
https://github.com/Kureev/react-native-navbar/blob/master/index.js
https://github.com/Kureev/react-native-side-menu/blob/master/index.js
https://github.com/rt2zz/react-native-drawer/blob/master/index.js
https://github.com/MattFoley/react-native-paypal/blob/master/ReactPaypal/index.ios.js
https://github.com/jmstout/react-native-viewport-units/blob/master/viewport-units.js
https://github.com/devfd/react-native-geocoder/blob/master/index.js
https://github.com/naoufal/react-native-activity-view/blob/master/ActivityView.ios.js
https://github.com/naoufal/react-native-speech/blob/master/SpeechSynthesizer.ios.js
https://github.com/corymsmith/react-native-icons/blob/master/SMXTabBarIOS.ios.js
https://github.com/oblador/react-native-keychain/blob/master/index.ios.js
https://github.com/almost/react-native-sqlite/blob/master/sqlite3.ios.js
https://github.com/jeanregisser/react-native-popover/blob/master/Popover.js
https://github.com/johanneslumpe/react-native-fs/blob/master/FS.ios.js
https://github.com/lwansbrough/react-native-camera/blob/master/Camera.ios.js
https://github.com/christopherdro/react-native-calendar/blob/master/CalendarIOS.js
https://github.com/naoufal/react-native-touch-id/blob/master/TouchID.ios.js
https://github.com/timfpark/react-native-location/blob/master/RNLocation.ios.js
https://github.com/darylrowland/react-native-remote-push/blob/master/RemotePushIOS.js
https://github.com/Shuangzuan/RCTRefreshControl/blob/master/RCTRefreshControl.ios.js
https://github.com/frostney/react-native-piechart/blob/master/RNPieChart.ios.js
https://github.com/jmstout/react-native-TouchableSetActive/blob/master/TouchableSetActive.js
https://github.com/lelandrichardson/react-native-parallax-view/blob/master/lib/ParallaxView.js
https://github.com/GertjanReynaert/react-native-device/blob/master/Device.js
https://github.com/lelandrichardson/react-native-segmented-view/blob/master/lib/SegmentedView.js
https://github.com/veddermatic/react-native-multipicker/blob/master/vedder-picker.ios.js
https://github.com/onefold/react-native-chart/blob/master/Chart.ios.js
https://github.com/kirkness/react-native-fs-modal/blob/master/index.js
https://github.com/stefalda/ReactNativeLocalization/blob/master/LocalizedStrings.js
https://github.com/leecade/react-native-swiper/blob/master/src/index.js
https://github.com/bulenttastan/react-native-list-popover/blob/master/ListPopover.js
https://github.com/jsierles/react-native-audio/blob/master/Audio.ios.js
https://github.com/lwansbrough/react-native-ab/blob/6009a1e31deb787ae31cedc62d762d33d800e076/Tracking.js
https://github.com/johanneslumpe/react-native-selectablesectionlistview
https://github.com/pjjanak/react-native-viewport/blob/master/Viewport.ios.js
https://github.com/lifuzu/ReactNativeBarcodeScanner/blob/master/BarcodeScanner.js
https://github.com/voronianski/react-native-effects-view/blob/master/index.ios.js
https://github.com/sheebz/react-native-headers/blob/master/index.js
https://github.com/yosit/react-native-twitter-login/blob/master/index.ios.js
https://github.com/khanghoang/RNSideMenu/blob/master/index.js
https://github.com/react-uikit/react-uikit
https://github.com/pjjanak/react-native-nested-stylesheets/blob/master/src/NestedStyleSheet.js
https://github.com/appintheair/react-native-looped-carousel/blob/master/index.js

Notes

  • ReactPropTypes is often required, can just be used via React.PropTypes
  • Tons of people just do require('NativeModules') (probably my fault:
    http://brentvatne.ca/facebook-login-with-react-native/)
  • ReactChildren is sometimes directly required, could just do React.Children

@ide
Copy link
Contributor

ide commented Jul 4, 2015

Very comprehensive @brentvatne. Looking over your candidates for inclusion here are some of my thoughts:

  • Dimensions - already exported on Thursday I believe
  • Platform - already exported
  • flattenStyle - not sure about this one. exposing the Registry opens up a lot of doors instead.
  • ViewStylePropTypes - not sure at the moment. leaning towards holding back for now since PropTypes aren't required for a component
  • TextStylePropTypes
  • LayoutPropTypes - leaning towards yes
  • StyleSheetPropType
  • ReactNativeViewAttributes - is this necessary anymore?
  • NativeMethodsMixin - not sure it's good to expose such an implementation detail right now
  • deepDiffer (and other differs) - probably doesn't make sense to export this unless there is some use case where a module needs to use the exact same algorithm as RN
  • precomputeStyle - probably, but maybe namespaced under StyleSheet
  • NavigatorNavigationBarStyles - this is such "leaf node" code that is subject to change rapidly. people should copy & paste
  • StyleSheetRegistry - yes, perhaps StyleSheet.Registry
  • StyleSheetValidation - maybe

dsibiski added a commit to dsibiski/react-native-userdefaults-ios that referenced this issue Jul 6, 2015
@dsibiski
Copy link
Contributor

dsibiski commented Jul 6, 2015

@brentvatne Thanks again for pointing this out. 👍

@mattmo
Copy link

mattmo commented Jul 7, 2015

Yes, this is a great list! This is a minor issue but it seems that cssVar should be removed from this example, given it's no longer exported (and probably shouldn't be).

I'm going to file a bug for it to be removed from react-native-navbar here:

https://github.com/Kureev/react-native-navbar/blob/master/index.js#L15

In general, are there any tests in place that ensure the files in the example directory perform properly? I noticed the travis config but didn't dive into the details for how it's used.

@brentvatne
Copy link
Collaborator Author

Great point @mattmo - it seems like Libraries/Utilities/CSSVarConfig.js and Libraries/Utilities/cssVar.js probably don't belong in OSS, what do you think @ide?

Good spot there with react-native-navbar too! 😄

@brentvatne
Copy link
Collaborator Author

In general, are there any tests in place that ensure the files in the example directory perform properly? I noticed the travis config but didn't dive into the details for how it's used.

There are, but they don't cover every example. The UIExplorer examples that are specifically tested via snapshots are Layout, Slider, Switch, TabBar, Text, and View. Other functionality (but not the specific screens in UIExplorer) are tested via unit tests and JS integration tests.

@brentvatne
Copy link
Collaborator Author

As per #1896 (comment) I think that Subscribable should also be added to the exports

@ide
Copy link
Contributor

ide commented Jul 7, 2015

it seems like Libraries/Utilities/CSSVarConfig.js and Libraries/Utilities/cssVar.js probably don't belong in OSS, what do you think @ide?

Yep they look unused.

@brentvatne
Copy link
Collaborator Author

ReactNativeART appears to be missing as well

@orktes
Copy link

orktes commented Jul 9, 2015

What about TextInputState?

I often use

TextInputState.blurTextInput(
  TextInputState.currentlyFocusedField()
);

to blur input & dismiss keyboard if I don't have ref to what is currently focused. This is handy for example when pushing a new route to Navigator or in the Navigator onWillFocus event.

@brentvatne
Copy link
Collaborator Author

That's pretty nifty @orktes, cc @vjeux ^

@chirag04
Copy link
Contributor

I could never understand TextInputState. Thanks for the example @orktes. Seems really useful. @brentvatne I think Input State should be documented. I don't fully understand it to PR.

@sahrens
Copy link
Contributor

sahrens commented Jul 18, 2015

We should definitely try to minimize the number of top-level things exported off of react-native.

We can hang TextInputState off of TextInput, and similar for other stuff like StyleSheet utilities.

What do people use precomputeStyle for?

Stuff like ViewStylePropTypes shouldn't be necessary because you can usually just use View.propTypes.style.

@brentvatne
Copy link
Collaborator Author

@sahrens - if you need to use setNativeProps with transforms, you'll need to use precomputeStyle before passing them in to setNativeProps. We could instead modify setNativeProps to run any transforms through that filter automatically.

Agreed re: View.propTypes.style and hanging TextInputState and such off of their parent components.

@ide
Copy link
Contributor

ide commented Jul 18, 2015

Let's expose StyleSheet.Registry then. Any pitfalls to doing so, keeping future StyleSheet optimizations in mind?

About ViewStylePropTypes -- I imagine people extend it when their components accept more styles than plain Views, similar to what Text does.

@vjeux
Copy link
Contributor

vjeux commented Jul 18, 2015

@brentvatne that's a bug, setNativeProps does call precomputeStyle: https://github.com/facebook/react-native/blob/master/Libraries/ReactIOS/NativeMethodsMixin.js#L105

@sahrens
Copy link
Contributor

sahrens commented Jul 18, 2015

What are folks using StyleSheet.Registry for?

On Jul 18, 2015, at 7:08 PM, Christopher Chedeau notifications@github.com wrote:

@brentvatne that's a bug, setNativeProps does call precomputeStyle: https://github.com/facebook/react-native/blob/master/Libraries/ReactIOS/NativeMethodsMixin.js#L105


Reply to this email directly or view it on GitHub.

@jeanregisser
Copy link
Contributor

Would be great to also expose Easing from react-native/Libraries/Animation/Animated/Easing.js.

@skevy
Copy link
Contributor

skevy commented Jul 29, 2015

@brentvatne
Copy link
Collaborator Author

I'm going to get a pull request in for an updated list of modules a week from today, please comment on here if a module that you are using has not been discussed above!

@brentvatne
Copy link
Collaborator Author

This still seems like something that is important to fix. Sorry for not getting that PR in, other things got in the way. @vjeux - would you like a PR for this still?

@vjeux
Copy link
Contributor

vjeux commented Sep 25, 2015

Yeah, we need to include it properly. Let's schedule a meeting on monday so we can go over all them in person and figure out a plan.

@brentvatne
Copy link
Collaborator Author

@vjeux - sounds good, I have a few meetings scheduled for Monday already but if 10am or 1pm work for you I can call in

@brentvatne
Copy link
Collaborator Author

Moving discussion to PR

brentvatne added a commit that referenced this issue Nov 5, 2015
Summary: - TextInputState as TextInput.State
- Touchable
- flattenStyle as StyleSheet.flatten
- ReactNativeART as ART

Original discussion in #1821
Closes #3308

Reviewed By: sebmarkbage

Differential Revision: D2527152

Pulled By: javache

fb-gh-sync-id: 19d4ef9d4c0e6587b9f0793e1ca624aebb034f3b
MattFoley pushed a commit to skillz/react-native that referenced this issue Nov 9, 2015
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 issue Dec 24, 2015
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
@TuangWang
Copy link

after i update the react-native. I got this error, "undefined is not an object (evaluating 'React.PropTypes.bool')".

@facebook facebook locked as resolved and limited conversation to collaborators Jul 22, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests