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

Deprecate RecyclerViewBackedScrollView #11445

Closed

Conversation

tychota
Copy link
Contributor

@tychota tychota commented Dec 13, 2016

cc @brentvatne

potential reviewers @mkonicek and @kmagiera

Motivation for making this change.

The previous PR was closed : #11095 but the followup actions was never done

I reopened a really similar one so it get merged
RecyclerView is no more used at Facebook (according to previous PR)

According to @brentvatne, their were two motivations for RecyclerView:

  • ListView with ScrollView component used to bounce back on row insert, but this is now fixed
  • This made possible to implement certain performance improvements, but the maintenance cost was not worth the risk

With RN 0.37, the actual code in React Native make the app crash:

I spend one hour investigating this and did also require brent time at exponent slack. I think other people are struggling too.

Test plan

screen shot 2016-12-13 at 23 42 22

Code formatting

The code should be formatted correctly, as it is copy and paste of a core contributor and mainly comments.

Next steps

This won't prevent the app from crashing though, so if you prefer, I can rename the
Libraries/Components/ScrollView/RecyclerViewBackedScrollView.ios.js to Libraries/Components/ScrollView/RecyclerViewBackedScrollView.js and remove the android file

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @lebronJ and @tegon to be potential reviewers.

@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 Dec 13, 2016
componentWillMount: function() {
console.warn(
'RecyclerViewBackedScrollView is DEPRECATED and will be removed from React Native. ' +
'Please use a ListView which has removeClippedSubviews enabled by default so that ' +

Choose a reason for hiding this comment

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

semi: Missing semicolon.

@lacker lacker changed the title Depreciate RecyclerViewBackedScrollView Deprecate RecyclerViewBackedScrollView Dec 14, 2016
@lacker
Copy link
Contributor

lacker commented Dec 14, 2016

@mkonicek has been working on deprecating stuff and generally refactoring things out from the core library, so it'd be nice for him to take a look. One question I have is, when we deprecate things, are we doing the same thing for all sorts of deprecated code. Like yellow boxing, or what. It'll be a better developer experience if we are consistent.

@jack668000
Copy link

可不可以打中文

@brentvatne
Copy link
Collaborator

@lacker - this is basically @mkonicek's PR re-opened, using his same code for consistency.

We should land this or something similar because RecyclerViewBackedScrollView is currently broken :\

@mkonicek
Copy link
Contributor

Thanks for doing this. I was actually just going to remove it right away, I don't think there's a reason for anyone to be using this component? It should probably never have been open sourced :)

@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Dec 16, 2016
@facebook-github-bot
Copy link
Contributor

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

@tychota
Copy link
Contributor Author

tychota commented Dec 16, 2016

@mkonicek : thank for accepting this :)

However I think it is not enough, it won't prevent people using it to see the app crash (as @brentvatne said, this is currently broken), with cryptic java logs :/

Moreover, some people are using RecyclerView as you can see in #10560, and they weren't aware that ListView has improve so much that the Recycler trick is no more useful.

I think, concerning this depreciation, it will be helpful to:

  1. merge the PR, it is probably not that useful but it may still help people looking in the code that RecyclerView is deprecated :)

  2. replace RecyclerViewBackedScrollView.ios.jsand RecyclerViewBackedScrollView.android.js by a component that could be something like this (not tested, just the concept):

class RecyclerViewBackedScrollView extends Component {
  componentWillMount() {
    console.warn(
      'RecyclerViewBackedScrollView is REMOVED from React Native. ' +
      'This is a simple wrapper to the ScrollView component.'
      'Please use a ListView which has removeClippedSubviews enabled by default so that ' +
      'rows that are out of sight are automatically detached from the view hierarchy.')
  }
  // keep the current API (at least scrollWithoutAnimationTo was not precent in recycler view ^^)
  scrollTo (
    y?: number | { x?: number, y?: number, animated?: boolean },
    x?: number,
    animated?: boolean
  ) {
    if !(typeof y === 'number') {
      ({x, y, animated} = y || {});
    }
    this.refs.scrollView.scrollTo({x: x || 0, y: y || 0, animated: animated !== false});
  }

  render() {
    // maybe sanitize props
    // maybe use function ref ;)
    return <ScrollView ref="scrollView" {...this.props} />
  }
}
  1. evangelize that ListView is much more mature and does not need special performance trick now, maybe in next release notes (cc @grabbou)

If you agree with the plan, I can commit to 1) with another PR.
What do you think ?

@tychota tychota deleted the deprecate/recyclersccrollview branch December 16, 2016 20:37
@brentvatne
Copy link
Collaborator

I think the component isn't needed at all anymore, based off of Krzysztof's comment @mkonicek

@tychota
Copy link
Contributor Author

tychota commented Dec 16, 2016

Let me PR the deletion ;)

@gold-duo
Copy link

ListView in the Android experience is terrible

DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this pull request Jan 4, 2017
Summary:
cc brentvatne

potential reviewers mkonicek and kmagiera

**Motivation for making this change.**

The previous PR was closed : facebook#11095 but the followup actions was never done

I reopened a really similar one so it get merged
RecyclerView is no more used at Facebook (according to previous PR)

According to brentvatne, their were two motivations for RecyclerView:
* ListView with ScrollView component used to bounce back on row insert, but this is now fixed
* This made possible to implement certain performance improvements, but the maintenance cost was not worth the risk

With RN 0.37, the actual code in React Native make the app crash:
- see facebook#10560

I spend one hour investigating this and did also require brent time at exponent slack. I think other people are struggling too.

**Test plan**

<img width="708" alt="screen shot 2016-12-13 at 23 42 22" src="https://cloud.githubusercontent.com/assets/13785185/21162483/dbeb642e-c18d-11e6-9c32-1fe73f1826c1.png">

**Code formatting**

The
Closes facebook#11445

Differential Revision: D4340640

Pulled By: mkonicek

fbshipit-source-id: 64c5cf060f2eb035d4d6199b30f0e73afc520180
@janiokq
Copy link

janiokq commented Nov 22, 2018

If it's a performance problem caused by listview
You can try this library
react-native-nlist

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants