Stop using legacy Context API for styles (continued) #4199
Conversation
|
I also removed the tip commit that was based on Ray's recommendation for testing that things don't break, and which was the only thing making the old PR a draft. You can find that commit on my non-PR branch |
|
|
||
| const subscriptionPropertiesFromStream: $Shape<Subscription> = deepFreeze(eg.stream); | ||
|
|
||
| export const makeSubscription = (args: $Shape<Subscription> = {}): Subscription => |
There was a problem hiding this comment.
EDIT: No longer applies; #3946 was merged. 🎉
Copied from the previous PR thread at #4046 (comment):
Note: we may instead want to cherry-pick 947dd8d (not yet merged) from #3946.
chrisbobbe
force-pushed
the
pr-remove-context-styles-2
branch
from
f9bb18b to
9b8697a
Compare
There was a problem hiding this comment.
Thanks @chrisbobbe ! Glad to have this cleaned up.
A few small comments. The bulk of these commits LGTM already, so I'll also go ahead and merge those.
|
|
||
| const expectedState = [ | ||
| { | ||
| color: 'red', |
There was a problem hiding this comment.
There's some behavior the existing test is testing which the new one doesn't: this color property was present in the previous state, missing in the subscription in the action, but remains present in the result state. IOW the reducer isn't clobbering the details of that subscription.
Not sure that's the ideal right behavior in the first place. If we get an op: 'add' subscription event that includes a stream we already had a subscription for, and especially if any of the data differed, then (a) I think that means something has already gone wrong and (b) if it does happen, we'd probably be best off doing the opposite of what this test tested for, and clobbering our existing clearly-stale data in favor of the new data. It's probably fine that we keep the code simple and effectively assume this case doesn't happen, but there's no need for the test to confirm that if it did happen we'd do the wrong thing.
But that kind of substantive change in what behavior is expected should be its own commit, separate from the big mostly-mechanical change to make things well-typed, so that it can be described in its own commit message. Could come either before or after.
There was a problem hiding this comment.
Thanks for catching this! I think that makes sense, but just to make sure:
But that kind of substantive change in what behavior is expected should be its own commit, separate from the big mostly-mechanical change to make things well-typed, so that it can be described in its own commit message. Could come either before or after.
The "substantive change" is the one that this commit currently does, not an additional one you're requesting, right? (I gather this from, "It's probably fine that we keep the code simple and effectively assume this case doesn't happen".)
If it comes before the commit to start type checking, I think it's most straightforward to just add color: 'red' to the subscription in the action.
If it comes after the type-checking commit, I think it's more complicated. There needs to be a "color" property (and all the other required properties) at the type-checking commit. To emulate the current expectation at the type-checking commit (that the sub in state does not get clobbered), we need to change some properties of the sub in the action while keeping the ID the same. Then, after that commit, the new commit would bring it back to the normal, "boring" sub with the same ID and properties.
Best, I guess, if things are as boring as possible all the way through, so I'll maybe do it before the type-checking commit?
| Our "Pure Component Principle" says `render` is a pure function of props, | ||
| state, *and context*. But `PureComponent` only checks for changes to props | ||
| and state, and skips re-render when just those two are unchanged. Doesn't | ||
| that open up bugs if just `this.context` changes? | ||
|
|
||
| [Yes, it | ||
| would](https://reactjs.org/docs/legacy-context.html#updating-context). For | ||
| this reason, when something provided in context is updated, we force the | ||
| entire React component tree under that point (in our usage of `context`, | ||
| this is nearly the entire tree) to re-render. This means we use `context` |
There was a problem hiding this comment.
I think this docs discussion sadly can't quite be deleted yet -- grepping for contextTypes, there's one place we still use it, namely to consume the intl context provided by the react-intl library. (In our TranslationContextTranslator component, which exists to... "translate" that legacy context into the new context API.)
And for that reason we do still use the key hack in TranslationProvider.
There was a problem hiding this comment.
Ah, right—I wonder if I wasn't thinking of this way back when I first created this PR. Anyway, do you think this means I should remove the marker that the PR fixes #1946?
| sparingly -- only for things where its benefit for code readability is very | ||
| large, and where updates are rare so we're OK with that global re-render. | ||
|
|
||
| In `StylesProvider`, for example, this is done with a `key`. |
There was a problem hiding this comment.
so s/Styles/Translation/ -- and maybe replace "for example" with a mention that this is now the one place we do this 🙂
src/styles/theme.js
Outdated
| |}; | ||
|
|
||
| export const themeColors: { [string]: ThemeColors } = { | ||
| export const themeColors: { [name: ThemeName | 'light']: ThemeColors } = { |
There was a problem hiding this comment.
Happy to have this be made more specific, but I was a bit confused by the commit message.
-
Instead of "fix annotation", the message can be more specific: namely that it makes the type (or the annotation) more specific.
-
I kept wanting to read the paragraph in the body as an explanation of the "why" for this change. But I couldn't see the connection -- and now I think the idea is that it's describing a future change we probably want to make to this code. Is that right?
There was a problem hiding this comment.
Instead of "fix annotation", the message can be more specific: namely that it makes the type (or the annotation) more specific.
I think the "fix" was aimed at Huh, looks like I was wrong about this! (doc)[string], which looks like a subtle mistake (though not a large one) — string isn't interpreted as a type; it's just what we were calling the indexer property.
Makes sense.
I kept wanting to read the paragraph in the body as an explanation of the "why" for this change. But I couldn't see the connection -- and now I think the idea is that it's describing a future change we probably want to make to this code. Is that right?
Mm, yeah (that's right), I'll make that clearer.
There was a problem hiding this comment.
In fact, maybe I'll move it to a code comment on the ThemeName type declaration in src/reduxTypes.
|
Merged 11/15 commits: |
Currently, this test confirms that the reducer isn't clobbering the details of a subscription. It does this in a way that doesn't look intentional; it may just be a consequence of not being forced to use well-typed (and therefore more likely to be representative) data in tests. The `color` property was - present in the previous state - missing in the subscription in the action - present (at the same value) in the result state. As Greg says in discussion [1], this isn't really an expectation that we want to have: """ If we get an `op: 'add'` subscription event that includes a stream we already had a subscription for, and especially if any of the data differed, then (a) I think that means something has already gone wrong and (b) if it does happen, we'd probably be best off doing the opposite of what this test tested for, and clobbering our existing clearly-stale data in favor of the new data. """ That being said, """ It's probably fine that we keep the code simple and effectively assume this case doesn't happen, but there's no need for the test to confirm that if it did happen we'd do the *wrong* thing. """ So, remove the unwanted expectation, and here, rather than in the same commit where we start type-checking this file, because it's a substantive change. [1] zulip#4199 (comment)
chrisbobbe
force-pushed
the
pr-remove-context-styles-2
branch
from
9b8697a to
30e07ca
Compare
|
OK, just resolved conflicts and pushed my changes! I responded your comments above; in particular, what do you think about this:
edit: Having expanded on the doc in |
chrisbobbe
force-pushed
the
pr-remove-context-styles-2
branch
2 times, most recently
from
a150112 to
30e3485
Compare
OK, I just pushed a commit that does this! Manual testing showed that the language is live-updated, and logging shows the components aren't being remounted (just rerendered). We get the updates we want, and no more |
There was a problem hiding this comment.
OK, I just pushed a commit that does this! Manual testing showed that the language is live-updated, and logging shows the components aren't being remounted (just rerendered). We get the updates we want, and no more
keyhacks at all, even here!
Amazing! So nice to have this fixed. 🙂
Some comments below -- looks like they're all about comments, docs, and commit messages at this point.
docs/architecture/react.md
Outdated
| As long as you use the [current Context | ||
| API](https://reactjs.org/docs/context.html) instead of the [legacy | ||
| one](https://reactjs.org/docs/legacy-context.html), there's not much | ||
| to worry about here. From the doc for the current one: |
There was a problem hiding this comment.
This sure sounds like there is something to worry about here, even when using the current Context API. Is there? AFAICT there isn't. But it'd be good to settle on an answer we're confident of. 🙂
docs/architecture/react.md
Outdated
| > The propagation from Provider to its descendant consumers (including | ||
| > [`.contextType`](https://reactjs.org/docs/context.html#classcontexttype) | ||
| > and | ||
| > [`useContext`](https://reactjs.org/docs/hooks-reference.html#usecontext)) | ||
| > is not subject to the shouldComponentUpdate method, so the consumer is | ||
| > updated even when an ancestor component skips an update. | ||
| (I think word "even" means to emphasize "ancestor" -- i.e., it's not | ||
| only the case that a consumer's *own* `shouldComponentUpdate` is | ||
| ignored, but that, remarkably, so is the `shouldComponentUpdate` of | ||
| ancestor components. I think it's just as remarkable that a |
There was a problem hiding this comment.
After staring at this for a minute, I think what's happening in that sentence is that the "so" isn't really appropriate. That is, it's trying to say two things:
- a consumer will be updated regardless of what its shouldComponentUpdate says
- a consumer will be updated regardless of whether its ancestors are updated, vs. skip an update.
The latter point isn't really about shouldComponentUpdate in the end, because those ancestors may not be consuming the context and so the first point may not apply to them -- they may really skip updating. It's saying that the consumer will update regardless of whether they do.
There was a problem hiding this comment.
Mm, you're right, I think this whole section can be made clearer; I'll rework it.
The latter point isn't really about shouldComponentUpdate in the end...
As I understand it: true, some of those ancestors may skip an update—but if they do, their shouldComponentUpdates are the reason why. They might not themselves consume the context, but shouldComponentUpdate can (hint to) skip an update based on nothing, or on anything it has available at the time.
docs/architecture/react.md
Outdated
| The legacy Context API is | ||
| [declared](https://reactjs.org/docs/legacy-context.html#updating-context) | ||
| fundamentally broken because `shouldComponentUpdate` and | ||
| `PureComponent`s can block updates. |
There was a problem hiding this comment.
If the answer about the current Context API is indeed that there's nothing to worry about, then this doc should avoid raising doubts about it. One step in doing that is that these paragraphs about the legacy API should have their own subheading, to keep them separate for the reader.
There was a problem hiding this comment.
I'll push a revision where the doubts are (hopefully) clearer; anyway, I use a concrete example. Then we can decide if it's worth mentioning at all, or at such length.
Currently, this test confirms that the reducer isn't clobbering the details of a subscription. It does this by making the `color` property - present in the previous state - missing in the subscription in the action - present (at the same value) in the result state. As Greg says in discussion [1], this isn't really an expectation that we want to have: """ If we get an `op: 'add'` subscription event that includes a stream we already had a subscription for, and especially if any of the data differed, then (a) I think that means something has already gone wrong and (b) if it does happen, we'd probably be best off doing the opposite of what this test tested for, and clobbering our existing clearly-stale data in favor of the new data. """ That being said, """ It's probably fine that we keep the code simple and effectively assume this case doesn't happen, but there's no need for the test to confirm that if it did happen we'd do the *wrong* thing. """ So, remove the unwanted expectation, and do it here, rather than in the same commit where we start type-checking this file, since it's a substantive change. [1] zulip#4199 (comment)
chrisbobbe
force-pushed
the
pr-remove-context-styles-2
branch
3 times, most recently
from
4c40cd8 to
73d8651
Compare
|
OK, just pushed my latest revision, fixing the straightforward things and hopefully clarifying the things about context. |
Currently, this test confirms that the reducer isn't clobbering the details of a subscription. It does this by making the `color` property - present in the previous state - missing in the subscription in the action - present (at the same value) in the result state. As Greg says in discussion [1], this isn't really an expectation that we want to have: """ If we get an `op: 'add'` subscription event that includes a stream we already had a subscription for, and especially if any of the data differed, then (a) I think that means something has already gone wrong and (b) if it does happen, we'd probably be best off doing the opposite of what this test tested for, and clobbering our existing clearly-stale data in favor of the new data. """ That being said, """ It's probably fine that we keep the code simple and effectively assume this case doesn't happen, but there's no need for the test to confirm that if it did happen we'd do the *wrong* thing. """ So, remove the unwanted expectation, and do it here, rather than in the same commit where we start type-checking this file, since it's a substantive change. [1] zulip#4199 (comment)
This includes the removal of the test 'on unrecognized action, returns input state unchanged' that is redundant with proper type checking: it's impossible to represent the input intended for the test in a way that passes our type checking, and the same type checking will guard against the input in live code.
As foreshadowed in this series, we get to remove the `key={theme}`
hack (along with some boilerplate to get things all working with the
old Context API).
Fixes: zulip#3994
The keys won't be just any string. Nice to tighten our types where we can.
Best seen in the ThemeContext declaration, below, it's inconsistent for one name to refer to "colors" and one not to. Furthermore, "colors" makes it sound like this is the only theme-related data that exists (which isn't necessarily true), or, worse, that there's some other theme-related data, besides the colors, that's found elsewhere, under a different name. This type encompasses all data about the theme, so, rename it to ThemeData. Just `Theme` was considered. `ThemeData` was chosen, not because of any semantics in Data, but because it's friendlier for newer contributors who might not be using their "whole-word" setting when doing a text search through the codebase. With `Theme`, a search without using that setting would newly contain a lot of noise from, e.g., ThemeName, ThemeProvider, ThemeContext, etc. Greg explains some useful searching strategies at zulip#4046 (comment).
In putting together a recent commit, I realized that this would be possible if our `TranslationContextTranslator` more carefully spoke the language of the old Context API. By making it a `Component` instead of a `PureComponent`, and making sure there aren't any disruptive ancestors between it and IntlProvider (in particular, there are *no* ancestors in between) it'll be updated when `intl` changes. It already speaks the new Context API as well as it needs to (i.e., nothing special has to happen to get its consumers to update), and we don't have any consumers that directly consume the old one straight from `react-intl`. One necessary tweak was to stop using `this._`, which is only set in the constructor. (Reminiscent of b530f6c, when we were removing the `key` hack for styles.) Fixes: zulip#1946
chrisbobbe
force-pushed
the
pr-remove-context-styles-2
branch
from
73d8651 to
68ca746
Compare
|
Thanks for the revisions! Merged. |
| stack. If it were possible, it would be a concern: setting a short | ||
| interval to automatically toggle the theme, we see that the message | ||
| list's color scheme changes as we'd want it to, but we also see the | ||
| bad effects that `shouldComponentUpdate` returning `false` is meant to | ||
| prevent: losing the scroll position, mainly (but also, we expect, | ||
| discarding the image cache, etc.). |
There was a problem hiding this comment.
Huh interesting.
That seems like a bug in the implementation of WebView, really -- it sounds like it's responding inappropriately to a re-render, and not behaving properly in the lifecycle of a React component. As React starts treating shouldComponentUpdate as more of a hint and less of a strict directive, this is the kind of bug that'll be getting exposed. It's possible we'll end up having to deal with the bug ourselves, if nobody else runs into it harder and fixes it first.
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.
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.
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.
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.
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.
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.
2 participants
Fixes: #1946
Fixes: #3994
A continuation of #4046, which has gotten cluttered with several unrelated conversations (sorry!). The work being done here isn't as complicated as the length of those conversations might suggest.
Greg has already merged several commits from that older PR, as he lists at #4046 (comment):