Skip to content

Commit

Permalink
Fixed runtime error with ProgressBarAndroid
Browse files Browse the repository at this point in the history 
Reviewed By: yungsters

Differential Revision: D5857305

fbshipit-source-id: 2fc20a848fa4dce5c1ac3fb7e986536618e25548
  • Loading branch information
@facebook-github-bot
Brian Vaughn authored and facebook-github-bot committed Sep 19, 2017
1 parent 9e37352 commit ccddbf8
Showing 1 changed file with 25 additions and 37 deletions.
62 changes: 25 additions & 37 deletions Libraries/Components/ProgressBarAndroid/ProgressBarAndroid.android.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,14 @@
*/
'use strict';

var NativeMethodsMixin = require('NativeMethodsMixin');
var React = require('React');
var PropTypes = require('prop-types');
var ViewPropTypes = require('ViewPropTypes');
var ColorPropType = require('ColorPropType');
const ActivityIndicator = require('ActivityIndicator');
const ColorPropType = require('ColorPropType');
const PropTypes = require('prop-types');
const React = require('React');
const ReactNative = require('ReactNative');
const ViewPropTypes = require('ViewPropTypes');

var createReactClass = require('create-react-class');
var requireNativeComponent = require('requireNativeComponent');

var STYLE_ATTRIBUTES = [
const STYLE_ATTRIBUTES = [
'Horizontal',
'Normal',
'Small',
Expand All @@ -29,10 +27,10 @@ var STYLE_ATTRIBUTES = [
'LargeInverse',
];

var indeterminateType = function(props, propName, componentName, ...rest) {
var checker = function() {
var indeterminate = props[propName];
var styleAttr = props.styleAttr;
const indeterminateType = function(props, propName, componentName, ...rest) {
const checker = function() {
const indeterminate = props[propName];
const styleAttr = props.styleAttr;
if (!indeterminate && styleAttr !== 'Horizontal') {
return new Error('indeterminate=false is only valid for styleAttr=Horizontal');
}
Expand Down Expand Up @@ -64,10 +62,10 @@ var indeterminateType = function(props, propName, componentName, ...rest) {
* },
* ```
*/
var ProgressBarAndroid = createReactClass({
displayName: 'ProgressBarAndroid',
propTypes: {
class ProgressBarAndroid extends ReactNative.NativeComponent {
static propTypes = {
...ViewPropTypes,

/**
* Style of the ProgressBar. One of:
*
Expand Down Expand Up @@ -97,35 +95,25 @@ var ProgressBarAndroid = createReactClass({
* Used to locate this view in end-to-end tests.
*/
testID: PropTypes.string,
},

getDefaultProps: function() {
return {
styleAttr: 'Normal',
indeterminate: true
};
},
};

mixins: [NativeMethodsMixin],
static defaultProps = {
styleAttr: 'Normal',
indeterminate: true
};

componentDidMount: function() {
componentDidMount() {
if (this.props.indeterminate && this.props.styleAttr !== 'Horizontal') {
console.warn(
'Circular indeterminate `ProgressBarAndroid`' +
'is deprecated. Use `ActivityIndicator` instead.'
);
}
},

render: function() {
return <AndroidProgressBar {...this.props} />;
},
});
}

var AndroidProgressBar = requireNativeComponent(
'AndroidProgressBar',
ProgressBarAndroid,
{nativeOnly: {animating: true}},
);
render() {
return <ActivityIndicator {...this.props} animating={true} />;

This comment has been minimized.

Sign in to view

Copy link
@chirag04

chirag04 Sep 19, 2017

Contributor

@bvaughn this looks little bit odd to me. ActivityIndicator is a circular spinner and ProgressBarAndroid is the horizontal progress bar. am i missing something?

This comment has been minimized.

Sign in to view

Copy link
@bvaughn

bvaughn Sep 19, 2017

Contributor

Seems like the full commit message didn't make it through with this one, unfortunately.

Agreed that the UI is odd Unfortunately ProgressBarAndroid and ActivityIndicator both claimed the same native component previously ("AndroidProgressBar")- which violates an invariant since React uses the native component name to map to the view config. It looked to me like this has been broken for some time and no one noticed.

I chose to update ProgressBarAndroid to just use ActivityIndicator since the latter has both Android and iOS implementations. It wasn't an obvious "right" choice. It just seemed the lesser of bad choices.

I'm open for suggestions though!

This comment has been minimized.

Sign in to view

Copy link
@chirag04

chirag04 Sep 19, 2017

Contributor

Ah. i didn't realize that ActivityIndicator and ProgressBarAndroid shared the same native component. I guess it makes sense because "intermediate" flag governs if it will be circular or horizontal on the native ProgressBar component.

i guess it's fine. maybe a comment to avoid the confusion but thanks for explaining it. 👍

This comment has been minimized.

Sign in to view

Copy link
@bvaughn

bvaughn Sep 19, 2017

Contributor

Yeah. I don't know the history of this particular view. I just noticed it was broken when doing some Android testing earlier this week.

Thanks for asking for clarification though. I'm not very familiar with large parts of the react-native codebase so it's good to question 😅

This comment has been minimized.

Sign in to view

Copy link
@brentvatne

brentvatne Oct 17, 2017 •
edited
Loading

Collaborator

It turns out that this actually breaks ProgressBarAndroid for any styleAttr other than "Normal" due to the order of props here: https://github.com/facebook/react-native/blob/master/Libraries/Components/ActivityIndicator/ActivityIndicator.js#L143-L145

To repro this, render the following code inside of a new app created with react-native init and notice that you will see a spinner rather than a horizontal progress bar:

  render() {
    return (
      <View style={styles.container}>
        <ProgressBarAndroid
          styleAttr="Horizontal"
          color="red"
          progress={0.5}
        />
      </View>
    );
  }

In addition to the styleAttr bug, the wrapper View around the native component in ActivityIndicator would potentially break the ProgressBarAndroid styling in some apps.

Lastly, I don't think ProgressBarAndroid was actually broken in old versions despite the invariant that you noticed @bvaughn - we do a manual test of some core components on every Expo release and didn't encounter any issues with it.

Not sure if we should revert this or fix in a separate commit, what do you think?

This comment has been minimized.

Sign in to view

Copy link
@ide

ide Oct 17, 2017

Contributor

ProgressBarAndroid worked in older versions but a commit 1~2 months back broke it, leading to a redbox if you required both ProgressBarAndroid and ActivityIndicator I think, hence this necessary fix.

This comment has been minimized.

Sign in to view

Copy link
@bvaughn

bvaughn Oct 17, 2017

Contributor

A little more context on what "broke it"- React Native was recently changed to lazy-load view manager configurations from UIManager (so that UIManager could in turn lazily load view managers).

The status of a lazily-loaded view, and its configuration, is keyed by its native class name. Unfortunately ProgressBarAndroid and ActivityIndicator both claimed the same native component (AndroidProgressBar) which triggered an invariant.

If I recall, this only happened to work before because (a) we didn't have the invariant and (b) no application tried to use both ProgressBarAndroid and ActivityIndicator.

As I mentioned above, when I noticed this I chose to use ActivityIndicator since it was the only one that had both Android and iOS implementations. This wasn't a great solution really, just patching up something that was broken.
}
}

module.exports = ProgressBarAndroid;

0 comments on commit ccddbf8

Please sign in to comment.