[ScrollView] Remove mixins from ScrollView #22374

thymikee · 2018-11-21T21:10:49Z

As a part of #22301 it turned out that we need to first convert ScrollView to class component. As a first step to do so, here's removal of using mixins API, in favor of populating _scrollResponder field with ScrollResponder.Mixin (still used) methods.

Test Plan:

Played around with RNTester app and looks ok, but I'd like more eyes on it.

Changelog:

[ScrollView][Removed] – Remove mixins from ScrollView

cc @TheSavior

pull-bot · 2018-11-21T21:12:15Z

	Warnings
⚠️	📋 Release Notes - This PR appears to be missing Release Notes.

Generated by 🚫 dangerJS

elicwhite · 2018-11-21T22:55:55Z

Libraries/Components/ScrollView/ScrollView.js

+        );
+      }
+    }
+    this._scrollResponder.props = this.props;


This seems like it would be problematic if the props passed to ScrollView change

Yeah, this is pretty brittle.

One way you could get around this is to define a getter.

Object.defineProperty(this._scrollResponder, 'props', { get: () => { return this.props; } })

But I'm not sure if that's a good idea because it'll be difficult for flow to type check this.

RSNara

Thank you for this PR! 😁

I think most of this is good, but we should go one step further and turn this into a React.Component subclass. 🤔

RSNara · 2018-11-22T00:02:16Z

Libraries/Components/ScrollView/ScrollView.js

+        );
+      }
+    }
+    this._scrollResponder.props = this.props;


Yeah, this is pretty brittle.

RSNara · 2018-11-22T00:25:18Z

Libraries/Components/ScrollView/ScrollView.js

@@ -551,22 +551,33 @@ export type Props = $ReadOnly<{|
 */
 const ScrollView = createReactClass({
  displayName: 'ScrollView',
-  mixins: [ScrollResponder.Mixin],
-
+  _scrollResponder: {},


It might be a good idea to just turn this into a React.Component subclass right now:

const ScrollResponder = require('ScrollResponder'); function createScrollResponder(node: React.ElementRef<typeof ScrollView>): typeof ScrollResponder.Mixin { const scrollResponder = {... ScrollResponder.Mixin}; for (const key of Object.keys(ScrollResponder.Mixin)) { if (typeof scrollResponder[key] === 'function') { scrollResponder[key] = scrollResponder[key].bind(node); } } return scrollResponder; } class ScrollView extends React.Component<Props, State> { _scrollResponder: typeof ScrollResponder.Mixin = createScrollResponder(this); }

Then, we change all references to scrollResponder methods within ScrollView to use this._scrollResponder

RSNara · 2018-11-22T00:26:42Z

Libraries/Components/ScrollView/ScrollView.js

+    for (const key in ScrollResponder.Mixin) {
+      if (typeof ScrollResponder.Mixin[key] === 'function') {
+        this._scrollResponder[key] = ScrollResponder.Mixin[key].bind(
+          this._scrollResponder,


To avoid having to proxy props and state, you should just bind this to the component instance.

RSNara · 2018-11-22T00:27:06Z

Libraries/Components/ScrollView/ScrollView.js

+        );
+      }
+    }
+    this._scrollResponder.props = this.props;


One way you could get around this is to define a getter.

Object.defineProperty(this._scrollResponder, 'props', { get: () => { return this.props; } })

But I'm not sure if that's a good idea because it'll be difficult for flow to type check this.

RSNara · 2018-11-22T00:28:30Z

Libraries/Components/ScrollView/ScrollView.js

  getInitialState: function() {
+    this._createScrollResponder();


I'm not too sure it's a good idea to hack getInitialState to initialize the scrollResponder. What we really need is a constructor, and we can get that by converting this component to a class, which shouldn't take too much time. I think we should just do that instead. Besides, we're going to have to remove this code eventually when we convert to a class down the line anyways.

RSNara · 2018-11-22T00:33:44Z

Libraries/Components/ScrollView/ScrollView.js

@@ -676,7 +688,7 @@ const ScrollView = createReactClass({
   * @platform ios
   */
  flashScrollIndicators: function() {
-    this.getScrollResponder().scrollResponderFlashScrollIndicators();
+    this._scrollResponder.scrollResponderFlashScrollIndicators();


I think we should just change getScrollResponder() to return this._scrollResponder.

Not sure about that. According to the comments getScrollResponder() is used to e.g. fire scrollTo() which is not available in ScrollResponder.Mixin. Unless they're wrong (there's no actual usage of getScrollResponder() in this repo, so it may relate to internal FB callsites)

Good call. 😅

RSNara · 2018-11-22T00:34:16Z

Libraries/Components/ScrollView/ScrollView.js

      layoutHeight: null,
    };
  },

  UNSAFE_componentWillMount: function() {
+    this._scrollResponder.UNSAFE_componentWillMount();


this.getScrollResponder().UNSAFE_componentWillMount();

RSNara · 2018-11-22T00:46:08Z

Libraries/Components/ScrollView/ScrollView.js

@@ -586,6 +597,7 @@ const ScrollView = createReactClass({
  },

  componentWillUnmount: function() {
+    this._scrollResponder.componentWillUnmount();


this.getScrollResponder(). componentWillUnmount();

thymikee · 2018-11-22T09:06:09Z

Libraries/Components/ScrollView/ScrollView.js

+    for (const key in ScrollResponder.Mixin) {
+      if (typeof scrollResponder[key] === 'function') {
+        scrollResponder[key] = scrollResponder[key].bind(this);
+        (this: any)[key] = scrollResponder[key].bind(this);


Turning this to class component turned out pretty straightforward. However, problem with binding scrollResponder to ScrollView is that methods from mixin will refer to ScrollView context, while they were not assigned to the ScrollView instance, but to this._scrollResponder. To mitigate that, I added an ugly temporary hack that copies methods from mixin back to the ScrollView. Wonder if it makes sense to actually do only that and only put lifecycle methods (componentWillMount/componentWillUnmount) inside this._scrollResponder

Right. I completely overlooked the fact that mixin methods use this to access sibling methods. 😅

What I was worried about was mixin methods using component methods, or other methods defined in the mixins. Like, ScrollResponder.Mixin.scrollResponderGetScrollableNode, which calls this.getScrollableNode:

/** * Returns the node that represents native view that can be scrolled. * Components can pass what node to use by defining a `getScrollableNode` * function otherwise `this` is used. */ scrollResponderGetScrollableNode: function(): ?number { return this.getScrollableNode ? this.getScrollableNode() : ReactNative.findNodeHandle(this); },

this.getScrollableNode is implemented on ScrollView, if you look further down in the file:

getScrollableNode: function(): any { return ReactNative.findNodeHandle(this._scrollViewRef); },

But I think you took care of this with your update.

🤔

Wouldn't this make the component lifecycle methods in ScrollResponder.Mixin override the component lifecycle methods of the ScrollView? In other words, when react executes UNSAFE_componentWillMount, wouldn't it actually execute the UNSAFE_componentWillMount of ScrollResponder.Mixin?

Yes, I wanted to address that, but I left it in a, let's say, "request for comments" state and lost track on this issue – this was the reason I left the this._scrollResponder so we can differentiate on state and lifecycle of mixin vs component. This is what I meant while writing:

Wonder if it makes sense to actually do only that and only put lifecycle methods (componentWillMount/componentWillUnmount) inside this._scrollResponder

So I think it's gonna be safer to revert this change you landed locally and get this sorted here (unless you'd like to to fix it on your own while landing it, then be my guest :D)

RSNara

This looks good!

Thanks for addressing the changes so quickly! (And sorry, for getting back so late 😔). I'll pull the changes internally, test this out, (fix it up if need be), and land it within the next few days.

RSNara · 2018-12-03T19:07:40Z

Libraries/Components/ScrollView/ScrollView.js

+    for (const key in ScrollResponder.Mixin) {
+      if (typeof scrollResponder[key] === 'function') {
+        scrollResponder[key] = scrollResponder[key].bind(this);
+        (this: any)[key] = scrollResponder[key].bind(this);


Right. I completely overlooked the fact that mixin methods use this to access sibling methods. 😅

What I was worried about was mixin methods using component methods, or other methods defined in the mixins. Like, ScrollResponder.Mixin.scrollResponderGetScrollableNode, which calls this.getScrollableNode:

/** * Returns the node that represents native view that can be scrolled. * Components can pass what node to use by defining a `getScrollableNode` * function otherwise `this` is used. */ scrollResponderGetScrollableNode: function(): ?number { return this.getScrollableNode ? this.getScrollableNode() : ReactNative.findNodeHandle(this); },

this.getScrollableNode is implemented on ScrollView, if you look further down in the file:

getScrollableNode: function(): any { return ReactNative.findNodeHandle(this._scrollViewRef); },

But I think you took care of this with your update.

RSNara · 2018-12-03T19:14:41Z

Libraries/Components/ScrollView/ScrollView.js

+    for (const key in ScrollResponder.Mixin) {
+      if (typeof scrollResponder[key] === 'function') {
+        scrollResponder[key] = scrollResponder[key].bind(this);
+        (this: any)[key] = scrollResponder[key].bind(this);


🤔

Wouldn't this make the component lifecycle methods in ScrollResponder.Mixin override the component lifecycle methods of the ScrollView? In other words, when react executes UNSAFE_componentWillMount, wouldn't it actually execute the UNSAFE_componentWillMount of ScrollResponder.Mixin?

RSNara · 2018-12-03T19:15:53Z

Libraries/Components/ScrollView/ScrollView.js

@@ -676,7 +688,7 @@ const ScrollView = createReactClass({
   * @platform ios
   */
  flashScrollIndicators: function() {
-    this.getScrollResponder().scrollResponderFlashScrollIndicators();
+    this._scrollResponder.scrollResponderFlashScrollIndicators();


Good call. 😅

RSNara · 2018-12-03T19:21:14Z

Libraries/Components/ScrollView/ScrollView.js

+    const scrollResponder = {...ScrollResponder.Mixin};
+    for (const key in ScrollResponder.Mixin) {
+      if (typeof scrollResponder[key] === 'function') {
+        scrollResponder[key] = scrollResponder[key].bind(this);


Since you are copying all the methods from ScrollResponder to the component instance, why do we need to also create the scrollResponder here? Why not just call the scrollResponder methods directly off of the component instance? Is it because of flow?

facebook-github-bot

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

react-native-bot · 2018-12-26T18:40:04Z

@thymikee merged commit 221e2fe into facebook:master.

thymikee added 3 commits November 21, 2018 22:00

[ScrollView] Remove mixins usage

d14af47

revert getScrollResponder removal

38feec9

whitespace

2dfbbb8

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 Nov 21, 2018

react-native-bot added 🔶Components Component: ScrollView Component: View labels Nov 21, 2018

elicwhite reviewed Nov 21, 2018

View reviewed changes

RSNara requested changes Nov 22, 2018

View reviewed changes

convert to class component

8e3ae32

thymikee commented Nov 22, 2018

View reviewed changes

RSNara approved these changes Dec 3, 2018

View reviewed changes

facebook-github-bot reviewed Dec 3, 2018

View reviewed changes

facebook-github-bot closed this in 221e2fe Dec 26, 2018

facebook locked as resolved and limited conversation to collaborators Dec 26, 2018

react-native-bot added the Merged This PR has been merged. label Dec 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ScrollView] Remove mixins from ScrollView #22374

[ScrollView] Remove mixins from ScrollView #22374

thymikee commented Nov 21, 2018

pull-bot commented Nov 21, 2018

elicwhite Nov 21, 2018

RSNara Nov 22, 2018

RSNara Nov 22, 2018

RSNara left a comment

RSNara Nov 22, 2018

RSNara Nov 22, 2018

RSNara Nov 22, 2018

RSNara Nov 22, 2018

RSNara Nov 22, 2018

RSNara Nov 22, 2018

thymikee Nov 22, 2018

RSNara Dec 3, 2018

RSNara Nov 22, 2018

RSNara Nov 22, 2018

thymikee Nov 22, 2018 •

edited

Loading

RSNara Dec 3, 2018

RSNara Dec 3, 2018

thymikee Dec 3, 2018

RSNara left a comment

RSNara Dec 3, 2018

RSNara Dec 3, 2018

RSNara Dec 3, 2018

RSNara Dec 3, 2018

facebook-github-bot left a comment

react-native-bot commented Dec 26, 2018

[ScrollView] Remove mixins from ScrollView #22374

[ScrollView] Remove mixins from ScrollView #22374

Conversation

thymikee commented Nov 21, 2018

Test Plan:

Changelog:

pull-bot commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RSNara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee Nov 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RSNara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Dec 26, 2018

thymikee Nov 22, 2018 •

edited

Loading