msglist [nfc]: Separate out "single-page webview" code from "message list" code #5523
Conversation
gnprice
added
a-message list
P1 high-priority
server release goal
There was a problem hiding this comment.
Thanks! This LGTM; please merge at will. Possibly my investigation about baseUrl on iOS is worth memoizing in a code comment somewhere.
I actually wrote a draft branch in this direction back in 2020-02.
There was another draft in 2020-07, in PR #4173, "Allow viewing message edit history". From comparing the two, I think we're fine to merge yours as-is. With your PR in, and with my #5435 in ("messages state: Maintain edit_history to be reasonably current"), it could be a good time to revisit showing edit history. 🙂
| * Assumes `baseUrl` has the scheme `file:`. No actual file need exist at | ||
| * `baseUrl` itself, because the page is taken from the string `html`. |
There was a problem hiding this comment.
I see that assumption holds on Android because we hard-code a URL with file:.
I think it holds on iOS too. Instead of hard-coding the value, we grab it with iOS native code in our ZLPConstants.m file and send it over the RN bridge (starting in commit 762ce28).
-
We decided against pointing to a file served by the packager in development, and presumably such a reference would have used an
http:URL. -
Apple's doc for the API we ended up using doesn't exactly say the scheme is
file:, but it does say "file URL", as in "The file URL of the bundle’s subdirectory containing resource files.". -
Our Jest mock (in jest/jestSetup.js) for the
ZLPConstantsnative module usesfile:///private/var/containers/Bundle/Application/4DCD4D2B-F745-4C70-AD74-8E5F690CF593/ZulipMobile.app/
which looks like a real value we've actually observed at some point.
There was a problem hiding this comment.
Thanks for tracing that. I went and added a commit documenting that assetsUrl is a file URL and explaining why, based on this.
- Apple's doc for the API we ended up using doesn't exactly say the scheme is
file:, but it does say "file URL", as in "The file URL of the bundle’s subdirectory containing resource files.".
Yeah, I think by "file URL" they have to mean "URL whose scheme is file". I don't think there's another meaning that could make it make sense for that to be the phrase they choose.
| // TODO: This should ideally be a proper React component of its own. The | ||
| // thing that may require care when doing that is our use of | ||
| // `shouldComponentUpdate` in its caller, `MessageList`. |
There was a problem hiding this comment.
Yeah, testing might reveal some special care needed, and that seems fine to postpone with this TODO in place.
I'd hope we could just make a "proper React component" that doesn't add or subscribe to state that would cause it to rerender. Then, hopefully, it wouldn't rerender except when its parent, MessageList, rerenders. (Which we hope it never does, but that goes against React's instructions on shouldComponentUpdate—"This method only exists as a performance optimization. Do not rely on it to 'prevent' a rendering, as this can lead to bugs.")
In #4173, which attempted a similar refactor in 2020-07, I suggested we could help ensure that by using a "stateless function component" instead of a class component. Before React Hooks, IIUC all function components were understood to be "stateless". With Hooks, I'd say the thing to try (now or later) is a function component that doesn't call useState or other Hooks that can cause a rerender, like useContext, React Nav's useIsFocused, RN's useWindowDimensions, etc.
There was a problem hiding this comment.
I actually have a draft branch that does this, and then goes on to make MessageList itself a Hooks-using function component. 🙂 Hoping to finish that up and send as a PR today.
The namespace import `import * as React` will allow us to use the same `React.Foo` style for types as for values, and avoid the hackier `React$Foo` naming.
This is just as constant as webviewAssetsUrl and assetsUrl; and the only thing we do with either of those is use them in computing this. So move its definition up next to theirs, at the module toplevel.
This code has really quite a different character from the rest of what the MessageList component is doing, and even more so from what the rest of the render method is doing. In fact it isn't really even about the message list at all: it's about using a WebView to show a single page, with all navigation handled externally. In its current location, it puts a wide separation between different parts of the actual message-list logic, making it harder to read, understand, or modify. So, make both this code and the actual message-list code easier to read, understand, and modify by separating them out from each other. Also take the opportunity to write some jsdoc explaining what this is supposed to do.
Fewer layers of nested function expressions; stop having a conditional to sometimes return one function expression and sometimes another. The cost (such as it is) is that we always make the local variable `loaded_once`, even on Android where we won't need it. I rather think we can afford one extra Boolean variable, one per message-list screen.
This method sure is a lot shorter now that all the "stay on one page" logic has been factored out! Celebrate by tightening a bit of formatting to make it easier still to take in the whole thing at once.
|
Thanks for the review! Merging, with a couple more comment-only commits based on the discussion above. |
gnprice
force-pushed
the
pr-msglist-webview-refactor
branch
from
6b83d33 to
e7fa85a
Compare
2 participants
In
MessageList.jswe have quite a bit of code that's really not about the message list in particular at all -- it's infrastructural, security-hardening code for having a webview that stays at some given static HTML and can't navigate elsewhere. This code has a very different character from the rest of the file, where we're doing important things like deciding what to show in the message list and keeping it updated.By being in the midst of the actual message-list code, it gets in the way of reading, understanding, or modifying that code. Conversely, being entangled with the message-list code makes this code more complex to work with than it need be. Clarify things by separating it out.
I actually wrote a draft branch in this direction back in 2020-02. World events then intervened rather distractingly, and I hadn't picked it back up from the pile. For #5364 I expect to want to make other changes in the message-list code, so I went back and found the draft, rebased it, and completed the branch.