msglist: Convert MessageList to a function component with Hooks #5524

gnprice · 2022-10-19T19:18:55Z

A followup to #5523, as foreshadowed at #5523 (comment) . This is the next step toward the message-list changes I'll want to make for #5364.

The tricky part here is the way we have MessageList seize the baton from React when it comes to updating the UI to reflect changes in data, and go take care of those updates by other means.

With a class component for MessageList, we've accomplished this by implementing shouldComponentUpdate to prevent re-renders. This has always been a bit of a latent bug, because that method's docs disclaim any guarantee of always preventing a re-render. In this branch, we:

update some discussion around that use of shouldComponentUpdate to reflect changes that have already happened;
then make some small code cleanups;
dig into exactly what we're accomplishing by avoiding re-renders here -- to which the answer is, rerenders are fine but we're specifically avoiding recomputing the HTML text, both for performance and correctness -- and add comments explaining that;
then make the conversion!

This PR concentrates on the MessageListInner component-type that does most of the work; it doesn't touch the several layers of wrappers which build up around it the actual MessageList component-type. I plan to tackle those for the next PR in the series.

chrisbobbe

Thanks, this is a really nice improvement!! Small comments below.

chrisbobbe · 2022-10-20T09:42:59Z

src/reactUtils.js

 const NODE_ENV = process.env.NODE_ENV;

+/** A unique value for private use by useComputeOnce. */
+const useComputeOnce_sentinel = Object.freeze({});


You could use, e.g., Symbol('first render') (doc) if you want the value to contain a hint about what it means.

chrisbobbe · 2022-10-20T09:46:59Z

src/reactUtils.js

+  if (ref.current === useComputeOnce_sentinel) {
+    ref.current = calculateValue();
+  }
+  // $FlowIgnore[incompatible-cast]: value must have come from create()


nit: I think you mean calculateValue(), not create()?

Thanks, yeah -- that was the name in a previous draft.

chrisbobbe · 2022-10-20T09:52:07Z

src/reactUtils.js

+ * the case where it does.
+ */
+export function useComputeOnce<T>(calculateValue: () => T): T {
+  const ref = React.useRef(useComputeOnce_sentinel);


This implementation with useRef looks good.

An alternative that you might have considered is to pass a function to useState, for "Lazy initial state", then just never call the resulting setFoo function.

I don't prefer one way over the other; possibly useRef does the job more transparently.

Yeah, I think either of those is a good solution. There might be a reason to prefer one over the other if one knew more about React internals, but I don't know of such a reason.

chrisbobbe · 2022-10-20T10:17:46Z

src/webview/MessageList.js

-  webviewRef = React.createRef<React$ElementRef<typeof WebView>>();
-  sendInboundEventsIsReady: boolean = false;
-  unsentInboundEvents: WebViewInboundEvent[] = [];
+  const webviewRef = React.useRef<React$ElementRef<typeof WebView>>();


Hmm, I think both the current React doc and the nice new beta doc for useRef aren't perfectly clear about what happens when you don't pass an initialValue argument to useRef.

If it means .current begins its life as undefined, there could theoretically be a problem when it runs into the !== null check in sendInboundEvents.

I would explicitly pass null, as we do in useUncontrolledInput, for example:

const ref = useRef<React$ElementRef<typeof TextInput> | null>(null);

Possibly React.createRef() (what we used before this commit) initialized .current to null. I say that because of the one word "back" in the doc:

React will assign the current property with the DOM element when the component mounts, and assign it back to null when it unmounts

gnprice · 2022-10-26T21:21:10Z

I'll return to revise this sometime soon. In the meantime I just merged the two docs commits at the start of the branch, as:
0122e45 docs/react: Note this doc hasn't been updated for Hooks
9e829ac docs/react: Update shouldComponentUpdate / Context API discussion

because I realized I wanted to point to that doc in a chat thread and wanted the updated version.

gnprice · 2022-11-04T20:51:55Z

OK, revision pushed!

The main change in this revision is that now that I better understand how MessageList interacts with the theme (after all the discussion around #5527), and now that MessageList is future-proof for the theme changing -- it'll have the glitch where the user gets scrolled to the bottom, but will display a coherent color scheme rather than black-on-dark text -- I wanted to preserve that future-proofness.

That meant that instead of the behavior of useComputeOnce I wanted something a bit more complicated. Rather than give that try to write down a generalization for that, I just open-coded it, and left out the commit adding useComputeOnce.

gnprice · 2022-11-04T20:54:27Z

(I also manually tested that if you cherry-pick the dropped final commit of #5527, open a message list while in auto-light mode, then go to system settings and switch to dark mode: (a) the version with useComputeOnce gives the same broken behavior Tim Pearson saw when developing #5527, but (b) this revision correctly switches to a coherent dark mode, at the cost of scrolling to bottom.)

gnprice · 2022-11-04T22:32:44Z

Hmm, novel failure in CI!

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':app:mergeDebugNativeLibs'.
> A failure occurred while executing com.android.build.gradle.internal.tasks.MergeJavaResWorkAction
   > 2 files found with path 'lib/armeabi-v7a/libfbjni.so' from inputs:
      - /home/runner/.gradle/caches/transforms-3/831e8f1cc84cedfec1b6016a8e4402fa/transformed/jetified-react-native-0.71.0-rc.0-debug/jni
      - /home/runner/.gradle/caches/transforms-3/c1a3380a9cbf89fa6816a956bb03cc37/transformed/jetified-fbjni-0.3.0/jni
     If you are using jniLibs and CMake IMPORTED targets, see
     https://developer.android.com/r/tools/jniLibs-vs-imported-targets

That comes after spending several minutes apparently successfully doing much of the build.

… And I reproduce locally, and I reproduce locally at main.

I'll investigate a bit further and probably start a chat thread. In the meantime, seems independent of this PR.

[edit: chat thread; fix at #5535.]

This fixes a build failure which started happening today due to an operational mistake in React Native release management: facebook/react-native#35204 Happily Gradle gives us a way to more precisely pin down what we want it to do when finding dependencies, which makes the build robust to this and any similar issue. I've sent the same fix upstream for the template app: facebook/react-native#35208 See also where we spotted the issue in CI: zulip#5524 (comment) and discussion in chat: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/build.20failure.3A.20libfbjni.2Eso.20duplicated/near/1459787

gnprice · 2022-11-07T17:51:57Z

(I rebased this atop main after #5535 went in; CI is passing again.)

In docs/architecture/react.md we say we derive all our class components from React.PureComponent, with one exception, namely MessageList. From a grep, there are a handful of other components where we use simply React.Component instead. I'm not sure the difference has much practical effect on those components, but adjust them to follow our pattern for consistency. None of these components need special behavior in this respect: they don't implement shouldComponentUpdate, and just like for other components in our app the values we pass for their props are never mutated, only replaced with new values.

The API this is referring to is actually the modern context API, just the class-component form of it. The "legacy context API" is something else, involving a static property `contextTypes`. We haven't used that one for anything since commit 30e4d19, PR zulip#4199, back in 2020-07. Also mention what's now the most common alternative to withGetText: the useContext hook.

We got this `Props` type properly type-checked with ac97c91 and the series around that.

No need to pass it around separately where we're already passing the whole props object.

Just to simplify a bit the top level of this class declaration. Also delete the redundant type annotation, and narrow the lint-ignore to the particular rule it's meant for.

By having it take a single props object, rather than some props-like values as positional arguments.

From some experimentation, it turns out that re-rendering a WebView element is perfectly fine, just as one would hope if WebView is to be a well-behaved React component type. What isn't fine is re-rendering it with a *different `html` value*. Doing that will cause the page to be reloaded, so that any UI state inside it gets reset -- most notably, the user gets scrolled right back to their starting point (i.e. the first unread, or else the very end). Which is pretty reasonable on the WebView's part. So that means that the way our current logic succeeds at avoiding that problem isn't that it avoids re-rendering the WebView (though it does do that); it's that it avoids re-rendering the MessageList, and thereby avoids ever computing a new `html` in the first place. As a result, it's perfectly fine to have another component interposed here. Even if this new component gets re-rendered for some arbitrary reason, it'll use the `html` value it got from its parent, namely MessageList -- which we continue to take care not to re-render, so the `html` value still never changes.

The code already behaved correctly, because when we look at this property we only ever test it for truthiness, so void is just as good as false. But it wasn't really well-typed; apparently Flow looked the other way.

Most of this is a routine class-to-Hooks conversion. The interesting part is the lifecycle method `shouldComponentUpdate`; the part where it tracks changes to props turns into an effect, and the part where it blocks (or tries to) re-renders turns into forcibly memoizing the bulk of the old render method using a ref. As a bonus, this fixes a longstanding latent bug: React could have at any time chosen to go ahead and re-render the component even though we have a `shouldComponentUpdate` that always returns false. (The React docs stress that there's no guarantee on that score, that the method exists only for performance optimization.) That would cause us to recompute `html`, potentially taking some time. And if the resulting value had changed since the initial render, it'd cause the state in the WebView, including scroll position, to reset. With React Hooks, we can use refs to ensure that we never compute `html` a second time, and never change its value, just as we intend. (With a bit of future-proofing to ensure we continue displaying something coherent if the theme changes, which zulip#5533 will enable.)

This just represents inlining the definition of `usePrevious`, and then simplifying. We'll make use of this ref in the next commit.

chrisbobbe · 2022-11-10T04:17:08Z

Thanks, LGTM! Merged. Very glad to have this done.

gnprice added a-message list P1 high-priority server release goal Things we should try to coordinate with a major Zulip Server release. labels Oct 19, 2022

chrisbobbe reviewed Oct 20, 2022

View reviewed changes

gnprice force-pushed the pr-msglist-hooks branch 2 times, most recently from 6e44878 to 2effc11 Compare November 4, 2022 20:48

gnprice mentioned this pull request Nov 4, 2022

android: Fix build to get react-native only from local Maven repo #5535

Merged

gnprice force-pushed the pr-msglist-hooks branch from 2effc11 to 32b5de4 Compare November 5, 2022 00:24

gnprice mentioned this pull request Nov 7, 2022

Message list doesn't update when messages marked unread #5536

Closed

gnprice added 12 commits November 9, 2022 19:44

msglist [nfc]: Use actual MessageList props type in handleOutboundEvents

09d5381

We got this `Props` type properly type-checked with ac97c91 and the series around that.

msglist [nfc]: Simplify by getting _ from props more

4f4b62f

No need to pass it around separately where we're already passing the whole props object.

msglist [nfc]: Inline the onError callback

1148de8

Just to simplify a bit the top level of this class declaration. Also delete the redundant type annotation, and narrow the lint-ignore to the particular rule it's meant for.

msglist [nfc]: Make renderSinglePageWebView more render-like

e5283d3

By having it take a single props object, rather than some props-like values as positional arguments.

msglist [nfc]: Add more comments about inbound-events vs re-render

086bd7c

msglist [nfc]: Properly initialize a boolean

190676c

The code already behaved correctly, because when we look at this property we only ever test it for truthiness, so void is just as good as false. But it wasn't really well-typed; apparently Flow looked the other way.

msglist [nfc]: Maintain props as a ref

5785363

This just represents inlining the definition of `usePrevious`, and then simplifying. We'll make use of this ref in the next commit.

msglist: Tweak how handleMessage callback gets the props

a44dd53

chrisbobbe force-pushed the pr-msglist-hooks branch from 32b5de4 to a44dd53 Compare November 10, 2022 04:04

chrisbobbe merged commit a44dd53 into zulip:main Nov 10, 2022

gnprice deleted the pr-msglist-hooks branch November 10, 2022 04:27

gnprice mentioned this pull request Nov 10, 2022

msglist [nfc]: Convert the wrapper HOCs to more Hooks #5544

Merged

Uh oh!

msglist: Convert MessageList to a function component with Hooks #5524

msglist: Convert MessageList to a function component with Hooks #5524

Uh oh!

Conversation

gnprice commented Oct 19, 2022

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice commented Oct 26, 2022

Uh oh!

gnprice commented Nov 4, 2022

Uh oh!

gnprice commented Nov 4, 2022

Uh oh!

gnprice commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Nov 7, 2022

Uh oh!

chrisbobbe commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gnprice commented Nov 4, 2022 •

edited

Loading

chrisbobbe commented Nov 10, 2022 •

edited

Loading