[REVIEWABLE] Stop using legacy Context API for styles #4046

chrisbobbe · 2020-04-16T21:41:51Z

EDIT: This isn't fixed yet; I've closed this PR and opened #4199, as this one has gathered several irrelevant conversations.

Fixes: #1946
Fixes: #3994

If you get to the commit "StreamItem: Defend against empty strings for color on Subscription." and you're wondering how I might approach testing for this in subscriptionsReducer-test.js, let me know, and let's chat on CZO or video. It's pretty tangential to this PR, but I have a local branch exploring a way to thoroughly test that bad/unexpected server data in an Action doesn't fool a reducer into corrupting its piece of state — without having to repeat a lot of code from the existing n tests, where n is the minimum number of tests required to establish correct output given expected input in the Action. Basically, it uses jest.spyOn to replay all the calls to the reducer from those n tests, but with the passed Action transformed into a bad one (with a function you define once), and checks that the resulting state is clean (with a function you define once). A whole new interface like this will be less welcome where n is small, obviously.

rk-for-zulip

General notes:

I have not quite given this commit the detailed review it needs, but I have given it a first pass and identified a few issues. One thing in particular that I have not done is to compare the existing and replacement styles to ensure there are no discrepancies – except in the case of RawLabel, where I knew there was one and also knew exactly what to look for (q.v. infra).
Many commit messages refer to "the parent" without making it clear that the parent commit is meant. ("the previous commit" is probably a better phrasing, even if it's technically ambiguous.)
Many commit messages also have things like

As earlier, I'd be happy to put these in an instance field if preferred.

which won't be important after the PR is resolved, and probably shouldn't be in the commit message of a commit that's intended to be merged. It would be better to put this sort of thing in a comment in the PR itself. (Or, alternatively, reword the comment so that it still makes sense in three years.)
StreamItem [nfc]: Remove isSelected prop that is never passed.

For reference, this was introduced back in PR Improve Stream sidebar style. Fixes #171 #199. See here for a bit of background, including why this feature was left in. (TL;DR: a) apparently we used to mimic the webapp's layout more strongly, with the left and right sidebars being in drawers you could pull out from either side of the screen, and b) Boris wanted to reuse this capability for another feature. Those are both gone, though.)

Specific things that break for me:

The icons and right-chevrons on the Settings page. These do not change when the setting is toggled; instead they remain the same color they were at app startup. (I haven't looked into why this is.)
Any RawLabel. These do change when the theme changes – but they're black in night mode (well, #01141C), and a light grey in the default theme (#757575).
- Some experimentation suggests that this is actually a constant translucent color, rgba(0,0,0,0.54). Why this is the default text color for RCTText elements, I couldn't possibly guess.
The Android status bar smoothly transitions between its night- and default-mode colors, but the rest of the app just snaps from one style to the next. (This PR doesn't cause that, just makes it more noticeable. If it's nontrivial to fix, it may be best to file a separate issue.)

rk-for-zulip · 2020-04-20T20:11:11Z

src/__tests__/lib/exampleData.js

+  });
+};
+
+export const subscription: Subscription = makeSubscription();


This doesn't seem to be used anywhere.

rk-for-zulip · 2020-04-20T20:16:31Z

src/__tests__/lib/exampleData.js

+  args: {
+    stream?: Stream,
+    color?: string,
+    in_home_view?: boolean,
+    pin_to_top?: boolean,
+    audible_notifications?: boolean,
+    desktop_notifications?: boolean,
+    email_address?: string,
+    is_old_stream?: boolean,
+    push_notifications?: null | boolean,
+    stream_weekly_traffic?: number,
+  } = {},
+): Subscription => {


This could be more simply rendered as $Shape<Subscription>, I think – e.g.:

export const makeSubscription = (args: $Shape<Subscription>): Subscription => Object.freeze({ ...baseSubscription, ...args });

... where baseSubscription is of course some static data.

Also, unless you expect other tests to use this constructor, I'd suggest leaving it in subscriptionsReducer-test.js rather than in the shared-data file.

rk-for-zulip · 2020-04-20T20:20:57Z

src/subscriptions/__tests__/subscriptionsReducer-test.js

-      ]);
-    });
-
-    test('when no `subscriptions` data is given reset state', () => {


I'm not sure how this test is "redundant with proper typechecking".

(Consider an implementation that appends subscriptions provided by REALM_INIT to those in the subscriptions-state. This would pass typechecking and even the preceding test, but would fail this one.)

This sure makes sense.

In this commit, my reason for removing these two tests (and, whether or not it's right, I should have written it down) is simply that, in each of them, it's impossible to represent their test input without defying our type definitions. So, in live code, the same bad input will be caught by the type checker, making any Jest tests that use that input redundant. What do you think; is this wrong, in general?

On a closer look, something seems weird with the bit of the reducer this test is for. I would expect Flow to complain after the ||, where action.data.subscriptions is falsy —

case REALM_INIT: return action.data.subscriptions || initialState; // should see a Flow error, but there is none

— but it doesn't. An inspection of RealmInitAction, following the aliases all the way down, shows that data.subscriptions is not an optional property, nor is it unioned with void.

Aha, and, indeed, a type-checked version of this test passes Flow with no errors, which I definitely did not expect, following my reading of RealmInitAction:

test('when no `subscriptions` data is given reset state', () => { const initialState = deepFreeze([sub1]); const action = deepFreeze({ ...eg.action.realm_init, data: { ...eg.action.realm_init.data, subscriptions: undefined, // no problems here! Why? }, }); const expectedState = []; const actualState = subscriptionsReducer(initialState, action); expect(actualState).toEqual(expectedState); });

So, I guess, other questions:

Am I reading our type definitions correctly, that action.data.subscriptions must never be missing, and

If so, how can we get away with expressing it as missing, anywhere? Maybe there's a Flow bug with spread types affecting the InitialData type? And, I guess,

Assuming I'm reading the type definitions correctly, and that they are our best knowledge of what will come from the server, do we even need to defend against data.subscriptions being missing, in our live code?

So let me pull the emergency brake here, because I completely missed that this test did not have data: {... subscriptions: []}, but merely data: {}.

(I do think a test with the former would be valuable, and that it would be better to turn this test into that test than to eliminate it entirely; but that's a very different thing than what I said earlier. 😓)

subscriptions: undefined, // no problems here! Why?

Oh, wow. That's horrifying. Let me poke at that for a bit.

— found it. Your guess in point 2 was on the right track, although the bug is in spread values rather than in spread types.

Like several other Flow bugs involving spread, this is fixed in v0.111.1, which will be available to us in RN 0.62.

Ah OK, no problem! Thanks for confirming this is a known Flow bug; glad there's an eventual solution, and good to be aware of the problem in the meantime.

Please add a comment with that result, so we can memoize that archaeological work 🙂

I was just doing that as we speak. Thanks, Ray! 🙂

— as in, I was just putting it in a commit message. Would you say we should have a code comment as well?

I'm seeing a subtle lesson coming from this.

On the one hand, it was likely skepticism and caution, in healthy doses, that motivated adding the || initialState check, in several places — skepticism and caution are good motivators that often drive the best design / code choices.

...but || initialState is dead code, and dead code should not exist (particularly when it makes our types look funny), and it's not at all trivial to prove that it's dead. You can't just make it go away with some Boolean algebra, for example. While the check was well-intentioned, its net effect has been negative, causing us all to go and do these investigations.

So, I'll keep this case in mind when I try to add "crunchy shell" checks, in future, which might turn out to be dead code that takes work to uproot. I sincerely hope that 923e2b4 doesn't come back to bite us in this way; some small amount of context is in the latter half of #3966 (comment).

— as in, I was just putting it in a commit message. Would you say we should have a code comment as well?

Yeah, on the type definition. See examples in initialDataTypes.js (for the few types there that are commented...) and in modelTypes.js. That's a handy place to later look up what we know.

So, I'll keep this case in mind when I try to add "crunchy shell" checks, in future, which might turn out to be dead code that takes work to uproot.

Yeah, so I would describe this || initialState check not as part of a "crunchy shell" but rather as the kind of thing we end up with in the middle of the app as a result of a soft, non-crunchy shell:

In our JS code and the data it handles from HTTP requests it makes to the Zulip server, there are a lot of spots that would benefit from a crunchy shell -- a lot of spots where the code in the middle of the app either has random checks and conditionals scattered through, or bugs from the lack of those.

And yep, this is exactly the kind of thing that can then happen with such code :)

Yeah, so I would describe this || initialState check not as part of a "crunchy shell" but rather as the kind of thing we end up with in the middle of the app

It sounds like the same would apply to 923e2b4, no? From rereading the "crunchy shell" doc, it sounds like the ultimate goal is to get things thoroughly vetted in a layer before the data even reaches our Redux reducers. If doing these checks in a reducer is considered to be "in the middle of the app", then 923e2b4 is thoroughly guilty.

One small bug that story brings out is that we don't have realm_user_groups marked as optional in our API types, and we should:

export type InitialDataRealmUserGroups = {| realm_user_groups: UserGroup[], |};

along with I guess a comment that it was new in 1.8.0 (or whatever).

There's lots of other stuff in that initialDataTypes module that needs a review for similar questions; the bulk of it is entirely uncommented. That's well afield from this PR, though.

Opened as #4073.

It sounds like the same would apply to 923e2b4, no? From rereading the "crunchy shell" doc, it sounds like the ultimate goal is to get things thoroughly vetted in a layer before the data even reaches our Redux reducers. If doing these checks in a reducer is considered to be "in the middle of the app", then 923e2b4 is thoroughly guilty.

Yeah, it's true that isn't the ideal spot in the app for the check to be happening. It still feels better to me in the case of 923e2b4 than here. Maybe one reason is that for 923e2b4 we'd looked and determined that the guarantee we needed wasn't something clearly specified in the API; whereas here it is, and so adding the check gives me the sense that the author just hadn't bothered to determine what the API really was and base the code on that.

rk-for-zulip · 2020-04-20T21:00:35Z

src/subscriptions/subscriptionsReducer.js

+const ensureValidColor = (sub: Subscription): Subscription => {
+  if (sub.color === '') {
+    return { ...sub, color: HALF_COLOR };
+  } else {
+    return sub;
+  }
+};


We should have a robust, reusable predicate to tell whether a given string is a valid color.

I believe we do – or at least the core component thereof:

import Color from 'color'; const isColor = (s: string) => { try { Color(s); return true; } catch (e) { return false; } } const asColor = (s: string): Color | void => { try { return Color(s).toString(); } catch (e) { return; } }

This is an NPM library that we're already using in various places. It depends on color-string (by the same author), which might be easier to use directly, but isn't a direct dependency of ours at the moment.

Following from Greg's comment at #4046 (comment) I think we don't need this (nor the empty-string check in this first revision).

rk-for-zulip · 2020-04-20T21:58:48Z

src/common/Screen.js

@@ -5,7 +5,9 @@ import type { Node as React$Node } from 'react';
 import { StyleSheet, View, ScrollView } from 'react-native';
 import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet';

-import type { Context, Dimensions, LocalizableText, Dispatch } from '../types';
+import type { ThemeColors } from '../styles';


Addressing the commit message:

I'd be happy to move all of componentStyles to an instance field; I just think it's most natural for the non-theme-dependent styles to live together.

That's probably for the best, just for consistency's sake. If you're concerned about this not updating when the theme updates, that's something that should probably be addressed across the codebase...

Also, I don't think this component is even visible when the user goes into their settings to change the theme.

... and in particular, I would really prefer to never have to care whether or not this is true.

[...] Even so, I think this is the correct way to handle a value that, in theory, could change at any time.

What you might want to do, for testing purposes, is to set up a temporary component in (e.g.) AppEventHandlers which basically just does this:

while (true) { await sleep(2500); await dispatch(settingsChange({theme: 'night'})); await sleep(2500); await dispatch(settingsChange({theme: 'default'})); }

... and then browse through various screens with that running, to confirm that nothing breaks unexpectedly.

I've done exactly this as part of this review. Except for the things that already had problems with manual theme changes, everything seemed fine; I was even able to compose and send a message without having my work interrupted by the theme change.

Also, I don't think this component is even visible when the user goes into their settings to change the theme.

... and in particular, I would really prefer to never have to care whether or not this is true.

This is precisely what I meant by what follows the sentence you've quoted: "Even so, I think this is the correct way to handle a value that, in theory, could change at any time." And what prompted me to avoid the latent bug in my own code. (Though, as we've seen, I neglected to fix the pre-existing instances I'd been aware of and forgot about.)

What you might want to do, for testing purposes, is to set up a temporary component in (e.g.) AppEventHandlers which basically just does [...]

Thanks for this idea and example code, I'll use it before I resubmit. 🙂

rk-for-zulip · 2020-04-20T22:40:30Z

src/styles/theme.js

 export type ThemeColors = {|
  color: string,
  backgroundColor: string,
  cardColor: string,
  dividerColor: string,
 |};

-export const themeColors: { [string]: ThemeColors } = {
+export const themeColors: { [name: ThemeName | 'light']: ThemeColors } = {


I'm not aware of any reason that anyone should be using the name 'light' to access this theme. Is there any reason not to just replace light with 'default' here?

If you mean 'light' as it occurs in the one line I changed, here, yes: the object literal assigned to const themeColors contains a light property.

If you're referring to the light property in that object literal, below my diff, yeah, I don't see any reason this shouldn't just be default. Looking at 6d5cf60^, there used to be an object-as-map called themeNameToObject, but, at least at that commit, it was only used in one place, and, there, only an index of type ThemeName was used on it, and ThemeName has always and forever been 'default' | 'night'.

Hmm. Taking a look at Apple's HIG document on Dark Mode, though, it makes it sound like "default" semantics shouldn't apply to either theme at the application level, but rather at the device level:

In Settings, people can choose Dark Mode as their default interface style and schedule automatic changes between the appearance modes. Because people make these choices at a systemwide level, they generally expect all apps to respect their preferences.

Perhaps we might actually deprecate the "default" name, and use "night" or "light" everywhere (with our ThemeName type being 'light' | 'night' instead of 'default' | 'night' as it is currently), at or on the path to #4009.

Perhaps we might actually deprecate the "default" name, and use "night" or "light" everywhere (with our ThemeName type being 'light' | 'night' instead of 'default' | 'night' as it is currently), at or on the path to #4009.

That reasoning makes sense to me.

Just mentioned this plan at #4009 (comment).

rk-for-zulip · 2020-04-20T23:24:31Z

src/common/RawLabel.js

-    styles: () => null,
-  };
+  static contextType = ThemeContext;
+  context: ThemeColors;


This declares context, but then never uses it.

(See top-level comment for the consequences of this.)

gnprice · 2020-04-22T21:14:40Z

f465f5e StreamItem: Defend against empty strings for color on Subscription.

With an effort, which was due to the somewhat convoluted plumbing, I
just verified that, when we *do* pass a value for the `color` or
`backgroundColor` prop to StreamItem, it is always the `color`
property from a Subscription.

Thanks for pinning that down! That makes a lot of sense, but I think I'd never quite noticed that was the case. (Because of that same convoluted plumbing, at least in part.)

One quick step to help consolidate the gains from you having determined that would be: let's add some minimal jsdoc on this component and in particular document that color and backgroundColor, if present, must be the color property of a subscription to the stream.

That isn't as good as it would be to make that inherent in the way the API works, but it has some real value -- it sets a marker in the ground, facing both inward to the implementation and outward to the call sites, of what they should rely on and what would be a bug.

(And then see thoughts below on making that inherent in the way the API works.)

Therefore, on the assumption that this will always be the case (!),
it's possible to verify that we never get an empty string for one of
these props by defending against empty strings in
`subscriptionsReducer`,

This part isn't necessary. The purpose of the !== '' checks from 84977a9 wasn't to defend against an empty string -- it was from uncertainty about whether there was an intentional case of an empty string, in which we were intentionally treating that as a missing/nullish color. Those conditions were included so that if there was such a case, that commit wouldn't change the behavior. (The purpose of the commit was to help get us to @flow strict-local everywhere.)

With what you've learned above, there isn't such a case, so we can simply drop these conditions.

If we have a subscription with a color that's the empty string, or some other invalid color, then that'll be a bug which would be good to fix; but I don't think we need to build right now a system to defend against that class of bug.

It might be more robust to make an additional check at the
StreamItem component's level, since it's possible for any string
value to be passed.

I think the direction I'd be inclined to go for further improving this StreamItem code is to formalize the fact that it takes data from a subscription or stream, and the relationship of the two colors to that:

add a prop like subOrStream: Subscription | Stream;
then replace color and backgroundColor with something like an enum: colorStyle: 'background' | 'icon' | 'none', and have StreamItem use that plus subOrStream.color to pick the desired combination of backgroundColor, iconColor, and textColor.
Also take the opportunity to eliminate props like name, isPrivate, isMuted that can be inferred directly from subOrStream.

gnprice · 2020-04-22T21:39:13Z

commit 6149a7e
Author: Chris Bobbe [email protected]
Date: Thu Apr 16 13:28:37 2020 -0700

ThemeContext [nfc]: Rename to ThemeColorsContext.

As hinted by the rename of StylesProvider to ThemeColorsProvider (as
opposed to ThemeProvider), in the parent:

"Theme colors", not "theme", precisely names what this context
contains. The new name makes it more obvious, for example, that the
actual string identifier for the theme, 'default' or 'night', is not
to be found there.

Also, it seems most consistent with other variable names; this
inconsistency is best seen in the declaration, in the status quo:

```
export const ThemeContext: Context<ThemeColors> = React.createContext(themeColors.default);
```

I agree it'd be good for the two names to agree. It's unfortunate that this makes the name more verbose, though. And I kind of feel ThemeColorsContext is too specific -- like yes, all its properties at present are colors, but there could potentially be some other kind of data in a theme too.

Or for that matter even while there isn't -- this object is the whole data of the theme. "Theme colors context" sounds like there's some other part of the theme, apart from the colors, which is being left out and is to be found somewhere else.

Perhaps leave this ThemeContext, and rename the type to ThemeData?

edit: ... Or even just Theme.

chrisbobbe · 2020-04-22T21:55:51Z

Thanks for the review!

The purpose of the !== '' checks from 84977a9 wasn't to defend against an empty string -- it was from uncertainty about whether there was an intentional case of an empty string, in which we were intentionally treating that as a missing/nullish color.

But, regardless of the purpose, the effect of the !== '' checks was that it did defend against an eventual empty string from the server, meaning that, all along, we may have been given empty strings from the server without knowing it, right?

I was reminded of a situation over at #3966 (comment), where I figured it would be easier to add such a defense than to verify the server's behavior (and the local branch I mention above, #4046 (comment), experiments with this case):

I said:

[...] it seems reasonable that no event would have been emitted if there weren't sane data to transmit.

Can you confirm this is in fact the server's behavior?

The way to establish this most strongly is to read the server code at every commit, and while I remember doing a little of that, and concluding that this is somewhat likely to be true, I think it's much easier to do what you suggested, along the lines of the "crunchy shell":

Alternatively or additionally, we could make that part of the interface/contract of getAggregatedPresence, and its caller (the reducer) could just ignore an event with no data, treating it as a no-op.

@gnprice said:

If we have a subscription with a color that's the empty string, or some other invalid color, then that'll be a bug which would be good to fix; but I don't think we need to build right now a system to defend against that class of bug.

It may be that I'm over- (or, alas, under-) thinking things in this particular case. And I can't think of a good motivation for the server to give us an empty string there. So I'm fine to leave out the defense against an empty string, I just wanted to explain my thought process a bit more.

gnprice · 2020-04-23T03:24:42Z

I was reminded of a situation over at #3966 (comment), where I figured it would be easier to add such a defense than to verify the server's behavior

True! (and good question!)

I guess one point of difference between that situation and this one is: in that one, the PR added some logic that completely enforced the structure we needed.

Here, the structure we're assuming the color property on a subscription has isn't really just that it's nonempty -- it's that it's a valid color string we can pass to RN as a color value in a style prop, or to the Color library function Ray mentions above. (And I hope those two criteria agree. 🤷 ) So to build a nice crunchy-shell piece here, we'd want to do something like try to parse it with Color. Or perhaps something more structured than that, if we can determine a more specific API guarantee about the shape of it -- like parse it as the regexp #\d{6}.

The check against '' was necessary in case there was some part of our code that was misusing '' here as a synonym of null or undefined. We used to have a lot of places in the app that did that, and still have some. But fortunately we haven't generally done that in the API from the server, so I'm happy proceeding on the assumption that that isn't secretly the case here (and taking the risk that we'll deal with it from a bug report in the future, if it turns out it is.)

chrisbobbe · 2020-04-23T06:38:34Z

Here, the structure we're assuming the color property on a subscription has isn't really just that it's nonempty -- it's that it's a valid color string

Quite true. It’s a bit hard to follow in the GitHub UI, but Ray’s mention of the Color library function was a response to my statement of this fact, in f465f5e (and the kind of response I sought, by writing that last bit in the commit message; as discussed, a PR comment with a link would have been better). The regexp idea answers that nicely as well.

fortunately we haven't generally [misused '' as a synonym of null or undefined] in the API from the server

Also true. And in the sole case I know offhand where we do get an empty string from the server, when an empty value is meant, the server is moving toward the cleaner behavior of just dropping the property, IIRC (and this is easy to define in modelTypes.js). That’s the timezone property on UserOrBot. Which supports the idea that empty strings coming from the server isn’t a widespread or growing problem that we have to constantly scan the horizon for.

chrisbobbe · 2020-04-23T21:23:58Z

@gnprice, for the rest of your guidance in #4046 (comment):

f465f5e StreamItem: Defend against empty strings for color on Subscription.
With an effort, which was due to the somewhat convoluted plumbing, I
just verified that, when we *do* pass a value for the `color` or
`backgroundColor` prop to StreamItem, it is always the `color`
property from a Subscription.
Thanks for pinning that down! That makes a lot of sense, but I think I'd never quite noticed that was the case. (Because of that same convoluted plumbing, at least in part.)

One quick step to help consolidate the gains from you having determined that would be: let's add some minimal jsdoc on this component and in particular document that color and backgroundColor, if present, must be the color property of a subscription to the stream.

That isn't as good as it would be to make that inherent in the way the API works, but it has some real value -- it sets a marker in the ground, facing both inward to the implementation and outward to the call sites, of what they should rely on and what would be a bug.

[...]
I think the direction I'd be inclined to go for further improving this StreamItem code is to formalize the fact that it takes data from a subscription or stream, and the relationship of the two colors to that:

add a prop like subOrStream: Subscription | Stream;

then replace color and backgroundColor with something like an enum: colorStyle: 'background' | 'icon' | 'none', and have StreamItem use that plus subOrStream.color to pick the desired combination of backgroundColor, iconColor, and textColor.

Also take the opportunity to eliminate props like name, isPrivate, isMuted that can be inferred directly from subOrStream.

I really appreciate that you've pointed out one simpler, but still valuable action, and another that's more complete and correct, but more involved. As I hope to do most of the time, I tended toward the latter. 🙂

After an attempt at this, though, I think it's probably best to do the more involved version after (or perhaps as part of) #3767, rather than in this PR. While I didn't find anything to contradict my discovery about the color / backgroundColor props, above, it seems that things are so tangled that, looking at the (just three!) consumers of StreamList, going the way of a subOrStream prop (which I agree sounds reasonable) wouldn't really work unless we expanded it to something like subOrStreamOrPseudoSubOrPerhapsUnreadStreamItem. 😆 A separation of concerns like #3767 seems like it might be a good step to disentangle this.

For now, I'll add that bit of JSDoc.

chrisbobbe · 2020-04-24T18:02:48Z

@gnprice said:

I agree it'd be good for the two names to agree. It's unfortunate that this makes the name more verbose, though. And I kind of feel ThemeColorsContext is too specific -- like yes, all its properties at present are colors, but there could potentially be some other kind of data in a theme too.

That's reasonable. I'm not sure what other data, where the two themes are light and dark, but I'm fine to leave it open this way.

Or for that matter even while there isn't -- this object is the whole data of the theme. "Theme colors context" sounds like there's some other part of the theme, apart from the colors, which is being left out and is to be found somewhere else.

Hmm, indeed, this is a stronger argument for keeping ThemeContext as-is.

Perhaps leave this ThemeContext, and rename the type to ThemeData?

edit: ... Or even just Theme.

I'm inclined toward ThemeData over Theme (though I'd defer to your judgment if you disagree!) — not because of any semantics in Data, but because with just "Theme", a search for this type alias across the codebase would newly return a lot of noise. ThemeData is still nicely disambiguated from AppTheme, ThemeName, Theme(Data?)Provider (the new name for StylesProvider after this series), ThemeContext, and handleThemeChange, some of which (e.g., ThemeContext) aren't tightly bounded on the number of occurrences as development continues.

rk-for-zulip

... so apparently these two comments have been sitting here, "Pending", ever since I wrote them.

This is plausibly GitHub's fault, but only plausibly. Sorry. 😓

rk-for-zulip · 2020-04-21T23:21:17Z

src/subscriptions/__tests__/subscriptionsReducer-test.js

-      ]);
-    });
-
-    test('when no `subscriptions` data is given reset state', () => {


So let me pull the emergency brake here, because I completely missed that this test did not have data: {... subscriptions: []}, but merely data: {}.

(I do think a test with the former would be valuable, and that it would be better to turn this test into that test than to eliminate it entirely; but that's a very different thing than what I said earlier. 😓)

subscriptions: undefined, // no problems here! Why?

Oh, wow. That's horrifying. Let me poke at that for a bit.

rk-for-zulip · 2020-04-21T23:33:18Z

src/subscriptions/__tests__/subscriptionsReducer-test.js

-      ]);
-    });
-
-    test('when no `subscriptions` data is given reset state', () => {


— found it. Your guess in point 2 was on the right track, although the bug is in spread values rather than in spread types.

Like several other Flow bugs involving spread, this is fixed in v0.111.1, which will be available to us in RN 0.62.

chrisbobbe · 2020-04-24T21:39:09Z

Thanks for the review, @ray-kraesig, if I haven't said so already!

@ray-kraesig said:

Many commit messages refer to "the parent" without making it clear that the parent commit is meant. ("the previous commit" is probably a better phrasing, even if it's technically ambiguous.)

I agree that "the parent" is an unnecessarily vague shorthand for "the parent commit". I think "the parent commit" is about equal to "the previous commit" in our rebase-oriented workflow; do you think that's wrong? For a different but related concern, I've been avoiding "the child commit" since Greg pointed out why I should, here:

There's an asymmetry between a commit's message referring to a parent and a child: the parent is an immutable fact about the commit, whereas it can always gain more children. There's a new child any time you make a PR/topic branch and happen to start from that commit; there might even be a new child added to the permanent shared history, if we make a stable-release branch and happen to start it from that commit.

So for that reason I prefer saying "the next commit", "an upcoming commit", and so on, even though I often say "the parent commit" as well as things like "the preceding commit". Saying "next commit" or "upcoming commit" is still a bit indeterminate, but the terms match the indeterminacy, whereas "child" is jargon.

chrisbobbe · 2020-04-24T21:41:28Z

@ray-kraesig said:

The Android status bar smoothly transitions between its night- and default-mode colors, but the rest of the app just snaps from one style to the next. (This PR doesn't cause that, just makes it more noticeable. If it's nontrivial to fix, it may be best to file a separate issue.)

I'll make a new issue once the present issue is resolved, as fixing this will mean touching all of these components, as well as the ones that have already been using the new Context API.

gnprice · 2020-04-24T21:54:11Z

I'm inclined toward ThemeData over Theme (though I'd defer to your judgment if you disagree!) — not because of any semantics in Data, but because with just "Theme", a search for this type alias across the codebase would newly return a lot of noise. ThemeData is still nicely disambiguated from AppTheme, ThemeName, [...]

I don't have a strong preference between these two names, but on this point I do have a tip you might find useful for other things! Whatever search UI you're using should have an option to search for whole words only.

For rg which I use all the time, or git grep which I often use for things rg can't do, that's -w. The Action type is a good example to play with:

$ rg Action | wc -l
603

$ rg -w Action | wc -l
118

Sometimes for longer names I'll lazily type just part of it and leave off the -w, but as a habit I usually use -w.

Better yet is a search based on actually parsing the code, to find references that are really to the same thing and not to an unrelated thing that happens to have the same name. I regularly use Shift+F12 "Show References" in VS Code, which provides those semantics.

chrisbobbe · 2020-04-25T00:06:06Z

@gnprice said:

I don't have a strong preference between these two names, but on this point I do have a tip you might find useful for other things!

Yay, thanks for pointing this out! I haven't been in the habit of using the whole-word setting in VSCode, so I don't usually remember it when it would be helpful. (I will from now on, though.) I haven't really used rg at all; it looks quite helpful, and it's got some syntax highlighting, which is nice.

I very regularly use Cmd+Click, which I think is equivalent to Shift+F12, when I want to jump straight to a definition. In most other cases, I guess I want to cast a wide net, which might point to why I haven't really been using the whole-word setting.

Given that I use Cmd+Click, I'm not actually sure how much my own searches would benefit from it being ThemeData instead of Theme.

But since you said you don't strongly favor one over the other, my next revision has it as ThemeData, at least with newer contributors in mind, who might be unaware of all of the above tricks. I'll gladly switch it, though, if you change your mind.

rk-for-zulip · 2020-04-25T01:36:52Z

Many commit messages refer to "the parent" without making it clear that the parent commit is meant. ("the previous commit" is probably a better phrasing, even if it's technically ambiguous.)

I agree that "the parent" is an unnecessarily vague shorthand for "the parent commit". I think "the parent commit" is about equal to "the previous commit" in our rebase-oriented workflow; do you think that's wrong?

Oh, if there's a functional difference between the terms, it eludes me. It's only that:

"parent" has a natural dual of "child", yet the latter is (as Greg noted) best avoided in this context; and
"previous commit" is, as far as I can tell, more commonly seen in the wild.

I sincerely doubt I will ever feel the need to comment on any choice you should make between these two hereafter, regardless of what that choice is or even how consistently you choose either. 🙂

Yay, thanks for pointing this out! I haven't been in the habit of using the whole-word setting in VSCode, so I don't usually remember it when it would be helpful. (I will from now on, though.) I haven't really used rg at all; it looks quite helpful, and it's got some syntax highlighting, which is nice.

You may also find something along the lines of alias rgF='rg -n --color=always --heading' useful (as I do) for forcing terminal-style formatting and syntax highlighting even when piping ripgrep's output into a pager.

I did a search through the whole project for all occurrences of the string "StreamItem". For each place in the JSX where the component was created, I did the following checks: A) By looking at the code, confirmed that we're not passing that prop ourselves, except for once, in `StreamList`. The value passed there is based on `StreamList`'s prop `selected`, which is similarly unused (so, remove that too). B) By looking at the React Native documentation, confirmed that FlatList and SectionList (under which StreamItem appears, and which we don't implement ourselves) don't inject this prop secretly, as would possible with, e.g., `React.Children` and `cloneElement`. C) Confirmed (A) and (B) empirically, via the `react-devtools` UI, by checking that isSelected isn't there. This was introduced in ec64098, for PR zulip#199. As Ray points out [1], much more recent discussion [2] suggests that we knew it was dead code, but wanted to preserve it for future functionality. As Ray summarizes, """ a) apparently we used to mimic the webapp's layout more strongly, with the left and right sidebars being in drawers you could pull out from either side of the screen, and b) Boris wanted to reuse this capability for another feature. Those are both gone, though. """ [1]: zulip#4046 (review) [2]: zulip#3462

A response to // TODO: confirm these '' cases are irrelevant, and remove. , added in 84977a9. With an effort, which was due to the somewhat convoluted plumbing, I just verified that, when we *do* pass a value for the `color` or `backgroundColor` prop to StreamItem, it is always the `color` property from a Subscription. We'd like to add this to an explicit contract for these props. Greg reports [1] that the `!== ''` checks from 84977a9 were there because we weren't sure whether there was an intentional use of an empty string in our code, to stand in for a missing/nullish color. Now that we know that there isn't, we can remove the checks. Contracts for `isMuted` and `isPrivate` are similarly induced from the call sites. So, add a JSDoc to that effect, to (paraphrasing Greg [1] again) mark, for the sake of the implementation and the call sites, of what they should rely on and what would be a bug. As Greg points out, this isn't as good as making these requirements inherent in the way the API works. We'd like to use a prop like `subOrStream: Subscription | Stream` and glean data like `name` and `isMuted` from that. Then the nested ternaries in `iconColor` and `textColor` (which I'm still not sure I fully understand, even after some pure refactors in recent commits) can be much simplified with, e.g., a prop with something like an enum: `colorStyle: 'background' | 'icon' | 'none'`. But this better solution will be much more accessible after (or as part of) zulip#3767. Currently, among StreamItem's three call sites, its data isn't only provided by a Stream or a Subscription object, but also a "PseudoSubscription" (I'm not sure what that is) and an UnreadStreamItem, so a relatively simple `subOrStream` prop isn't feasible. [1]: zulip#4046 (comment)

…_INIT. Our `RealmInitAction` type is quite clear that `subscriptions` is always present. That type is *meant* to represent our best knowledge of what will come from the server, but it's important to verify. Greg did some digging [1] and reported that the `|| initialState` check appeared to be modeled on the solution to a problem that didn't, after all, affect `subscriptionsReducer`: """ Looks like that originates in 462215c. Following links, that was from zulip#2878 which fixed zulip#2859. The latter issue was that, in a _different_ reducer, we needed exactly this because the data in question was about a recent feature that some servers didn't support and so the property (namely `realm_user_groups`) would be missing. """ Ray also did some archaeological investigation to see if it was reasonable to trust that the server has never handed out `null` or another falsy value, and concluded [2], """ As of at least 7a930afa0 (2017-02-20), and probably much earlier, it should always send at least `[]` for `subscriptions` so long as `subscription` is specified as a desired event type in the `/register` call. (I'm pretty sure that's true both at that commit and in today's code; but there _are_ a lot of intermediate changes that I haven't checked fully, so I'm less sure it's been true the entire time.) """ [1]: zulip#4046 (comment) [2]: zulip#4046 (comment)

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).

chrisbobbe · 2020-04-25T01:44:30Z

OK, @ray-kraesig, @gnprice, so I've pushed my next revision, with a [DO NOT MERGE] commit at the tip (see below).

I believe I've considered and responded to everything here (there's been a lot; thanks, both of you!), either with code/commit message changes, or by replying here on the PR.

I've done the testing Ray recommended above, with the example in a Gist, auto-cycling between the themes every few seconds so you can explore the app and see that things are live-updating properly; this is included at the tip, tagged [DO NOT MERGE]. My hope in including this is that it's convenient for Greg (or you, if you've lost your working setup) to be able to test with this nonce component out-of-the-box. Everything looks as expected, for me.

Ray, I think you're inclined to maintain the invariant that any PR branch with a [DO NOT MERGE] commit should be treated entirely as a draft, with a [REVIEWABLE] tag in the title, as applicable. I've done both here — just know that that single commit is the only reason for these markers (EDIT: and, alas, the CI failures 😞 — I'll fix those if it's important to you, but I know you can run all tests on every candidate-for-mergeable commit, and see them pass, with git rebase -i --exec 'tools/test', which I've done.)

@ray-kraesig said:

One thing in particular that I have not done is to compare the existing and replacement styles to ensure there are no discrepancies [...]

I did another pass of this, since the RawLabel bug slipped through on the first revision.

chrisbobbe · 2020-04-25T01:53:21Z

@ray-kraesig said:

Oh, if there's a functional difference between the terms, it eludes me. It's only that:

"parent" has a natural dual of "child", yet the latter is (as Greg noted) best avoided in this context; and

"previous commit" is, as far as I can tell, more commonly seen in the wild.

Mm, true! And I guess I wasn't as tuned-in to the wild. I could see the argument for using "previous commit", to maintain consistency with (a) avoiding "child", and (b) that wild.

gnprice · 2020-04-25T03:08:59Z

I very regularly use Cmd+Click, which I think is equivalent to Shift+F12, when I want to jump straight to a definition. In most other cases, I guess I want to cast a wide net, which might point to why I haven't really been using the whole-word setting.

It looks like Cmd+click is "Go to Definition". That's also very handy (I use F12 for it, and do that often), but different -- "Go to References" finds all the places where the thing is used, or referred to, instead of just the (usually one) place it's defined.

Looks like you can get the latter from the right-click menu, as well as Shift+F12.

There's also "Find All References", which gets the same results as "Go to References" but puts the results in the sidebar. The docs don't say so, but that's also in the right-click context menu (as well as being at Shift+Alt+F12.)

chrisbobbe · 2020-04-25T03:42:07Z

It looks like Cmd+click is "Go to Definition". That's also very handy (I use F12 for it, and do that often), but different -- "Go to References" finds all the places where the thing is used, or referred to, instead of just the (usually one) place it's defined.

It doesn’t seem to be documented, but the behavior of “Cmd+Click” seems to vary, so that it shows “references” if you use it on a definition, and it shows “definition(s?)” otherwise. (I’m not at the computer at the moment or I’d double-check.)

gnprice · 2020-05-01T01:29:04Z

Thanks again!

I've gone and merged 10 of these commits, along with a small added commit at the end:
f9caf22 SmartUrlInput: Switch to new React Context API for theme colors.
a197c99 StreamItem: Switch to new React Context API for theme colors.
64e1422 miscStyles [nfc]: Remove unused color in miscStyles.
ecba735 StreamItem [nfc]: Readability refactor (no need to repeat object literals).
aa4f3ad StreamItem [nfc]: Refactor textColor to look more like iconColor above it.
be26c74 StreamItem [nfc]: Remove isSelected prop that is never passed.
452edbc StreamItem: Confirm '' cases are irrelevant, remove check, add JSDoc.
2a4d9ee StreamAutocomplete [nfc]: Rename streams to matchingSubscriptions.
67be6ac subscriptions reducer test: Test for subscriptions being empty array.
3faf5a4 subs reducer: Remove needless check for void subscriptions in REALM_INIT.
29c22e0 StreamItem [nfc]: Reorder props for logical grouping.

The only other adjustment I made is that I reordered "StreamAutocomplete [nfc]: Rename streams to matchingSubscriptions." from later in the branch. It mentions "the parent commit" and I'm pretty sure that's meant to refer to "StreamItem: Confirm '' cases are irrelevant, remove check, add JSDoc." (Hopefully I'm not mistaken about that!)

Still reviewing the rest, but this will help move things forward and also reduce the work of future rebases and reviews 🙂

I did a search through the whole project for all occurrences of the string "StreamItem". For each place in the JSX where the component was created, I did the following checks: A) By looking at the code, confirmed that we're not passing that prop ourselves, except for once, in `StreamList`. The value passed there is based on `StreamList`'s prop `selected`, which is similarly unused (so, remove that too). B) By looking at the React Native documentation, confirmed that FlatList and SectionList (under which StreamItem appears, and which we don't implement ourselves) don't inject this prop secretly, as would possible with, e.g., `React.Children` and `cloneElement`. C) Confirmed (A) and (B) empirically, via the `react-devtools` UI, by checking that isSelected isn't there. This was introduced in ec64098, for PR #199. As Ray points out [1], much more recent discussion [2] suggests that we knew it was dead code, but wanted to preserve it for future functionality. As Ray summarizes, """ a) apparently we used to mimic the webapp's layout more strongly, with the left and right sidebars being in drawers you could pull out from either side of the screen, and b) Boris wanted to reuse this capability for another feature. Those are both gone, though. """ [1]: #4046 (review) [2]: #3462

The last use of this was removed in the parent commit.

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). Use the same strategy as in recent commits, for Input and ChatScreen: put the constant parts of the style in an instance field, and compute the theme-dependent parts in the `render` method.

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). Use the same strategy as in recent commits, for Label, Input, and ChatScreen: put the constant parts of the style in an instance field, and compute the theme-dependent parts in the `render` method.

The last two uses of this were removed in the two previous commits.

Continuing the work of recent commits toward zulip#1946. These last few consumers are simply using `background` or `backgroundColor` from the old style context, and no theme-independent styles, so, fix them together in one commit. Then, to help verify that all consumers are removed: Atomically, in this commit, remove the default-exported function of `miscStyles`, and its only call site, at `stylesFromTheme`. `stylesFromTheme` is the single point where all the data is collected and distributed as `styles` via the legacy Context API. (A similar removal, for `navStyles`, is bfdde6f.)

Compute theme-dependent styles in the render method. As foreshadowed in a previous commit that touched src/common/Screen.js, fix the remaining latent bugs where the placement of theme-dependent styles in a class initializer, instead of the `render` method, meant that these styles don't live-update when the theme changes, unless the component is unmounted and a new instance is created to replace it. The `key={theme}` hack accomplished this replacement, so this bug hasn't been visible. But that hack will disappear in an upcoming commit, so this change needs to be made before then. These bugs were introduced in 51dd1b3: LineSeparator.js a4e0f23: OptionButton.js and OptionRow.js f6ddc2d: OptionDivider.js

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). So, also, remove a bit of documentation that explained the hack. The section was necessary, previously, but it hindered an easy understanding of the "Pure Component Principle" and our use of it, which is a main idea of that doc. Fixes: zulip#1946

We'll want to deprecate the 'default' name; as I explain on zulip#4009 [1], any 'default' semantics should only apply the device level, at least on iOS, not within the app. [1]: zulip#4009

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).

chrisbobbe · 2020-05-01T17:02:59Z

It mentions "the parent commit" and I'm pretty sure that's meant to refer to "StreamItem: Confirm '' cases are irrelevant, remove check, add JSDoc."

Yep, that's right! Thanks for the review and merging.

chrisbobbe · 2020-07-10T01:01:50Z

src/subscriptions/__tests__/subscriptionsReducer-test.js

+
+const subscriptionPropertiesFromStream: $Shape<Subscription> = deepFreeze(eg.stream);
+
+export const makeSubscription = (args: $Shape<Subscription> = {}): Subscription =>


Note: we may instead want to cherry-pick 947dd8d (not yet merged) from #3946.

With Flow v0.113.0, which we'll get in the RN v0.61 -> v0.62 upgrade (zulip#3782), red flags are raised about this test. Strangely [1], the test was allowed to expect the `zulipVersion` on an Account to be undefined. In fact, the type for it is `ZulipVersion | null`. The test is based on the false premise that a REALM_INIT action might not have a `zulipVersion` property. `RealmInitAction` states that it will always be there, as a ZulipVersion instance. So, delete the test. If we ever find that there's a legitimate case where it is missing, we'll fix the RealmInitAction type accordingly, and a new test can proceed from there. [1] I think the culprit is very likely facebook/flow#6715, which Ray reported as being fixed in Flow v0.111.1, at zulip#4046 (comment)

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).

chrisbobbe · 2020-07-20T18:47:21Z

I'm opening a new PR for the remaining commits, and closing this one, as it has gathered several (helpful! but alas) irrelevant conversations.

EDIT: Opened as #4199.

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).

With Flow v0.113.0, which we'll get in the RN v0.61 -> v0.62 upgrade (zulip#3782), red flags are raised about this test. Strangely [1], the test was allowed to expect the `zulipVersion` on an Account to be undefined. In fact, the type for it is `ZulipVersion | null`. The test is based on the false premise that a REALM_INIT action might not have a `zulipVersion` property. `RealmInitAction` states that it will always be there, as a ZulipVersion instance. So, delete the test. If we ever find that there's a legitimate case where it is missing, we'll fix the RealmInitAction type accordingly, and a new test can proceed from there. [1] I think the culprit is very likely facebook/flow#6715, which Ray reported as being fixed in Flow v0.111.1, at zulip#4046 (comment)

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).

rk-for-zulip reviewed Apr 21, 2020

View reviewed changes

rk-for-zulip reviewed Apr 24, 2020

View reviewed changes

chrisbobbe mentioned this pull request Apr 24, 2020

Automatically follow user's OS-level dark/light preference, take 1 #4009

Closed

chrisbobbe force-pushed the pr-remove-context-styles branch from e4647d9 to b14011d Compare April 25, 2020 01:44

chrisbobbe marked this pull request as draft April 25, 2020 01:44

chrisbobbe changed the title ~~Stop using legacy Context API for styles~~ [REVIEWABLE] Stop using legacy Context API for styles Apr 25, 2020

This was referenced Apr 27, 2020

api types: realm_user_groups is absent prior to v1.8.0. #4073

Merged

Stop using fake blank data at login; and restore #4040 #4078

Merged

Chris Bobbe added 10 commits April 30, 2020 18:29

miscStyles [nfc]: Delete unused input in miscStyles.

76c1b37

The last use of this was removed in the parent commit.

miscStyles [nfc]: Delete unused label.

a816d6e

The last two uses of this were removed in the two previous commits.

theme types: Fix themeColors annotation.

d9a8cb4

We'll want to deprecate the 'default' name; as I explain on zulip#4009 [1], any 'default' semantics should only apply the device level, at least on iOS, not within the app. [1]: zulip#4009

[DO NOT MERGE] testing theme stuffs

6b5f32e

gnprice force-pushed the pr-remove-context-styles branch from b14011d to 6b5f32e Compare May 1, 2020 01:29

chrisbobbe mentioned this pull request Jun 2, 2020

View uploaded files without needing to be logged in to the realm in the browser. #4089

Merged

chrisbobbe commented Jul 10, 2020

View reviewed changes

chrisbobbe mentioned this pull request Jul 13, 2020

Night mode toggle also switches to Home card #3994

Closed

chrisbobbe mentioned this pull request Jul 20, 2020

Bug with Night mode #4197

Closed

rk-for-zulip mentioned this pull request Jul 20, 2020

Upgrade to RN v0.62 #3782

Closed

chrisbobbe closed this Jul 20, 2020

chrisbobbe mentioned this pull request Jul 20, 2020

Stop using legacy Context API for styles (continued) #4199

Merged

chrisbobbe deleted the pr-remove-context-styles branch November 5, 2021 00:27


		const subscriptionPropertiesFromStream: $Shape<Subscription> = deepFreeze(eg.stream);

		export const makeSubscription = (args: $Shape<Subscription> = {}): Subscription =>

[REVIEWABLE] Stop using legacy Context API for styles #4046

[REVIEWABLE] Stop using legacy Context API for styles #4046

Conversation

chrisbobbe commented Apr 16, 2020 • edited Loading

rk-for-zulip left a comment

Choose a reason for hiding this comment

General notes:

Specific things that break for me:

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

chrisbobbe Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 24, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Apr 23, 2020 • 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

gnprice commented Apr 22, 2020

gnprice commented Apr 22, 2020 • edited Loading

chrisbobbe commented Apr 22, 2020 • edited Loading

gnprice commented Apr 23, 2020 • edited Loading

chrisbobbe commented Apr 23, 2020 • edited Loading

chrisbobbe commented Apr 23, 2020 • edited Loading

chrisbobbe commented Apr 24, 2020 • edited Loading

rk-for-zulip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Apr 24, 2020

chrisbobbe commented Apr 24, 2020

gnprice commented Apr 24, 2020

chrisbobbe commented Apr 25, 2020 • edited Loading

rk-for-zulip commented Apr 25, 2020

chrisbobbe commented Apr 25, 2020 • edited Loading

chrisbobbe commented Apr 25, 2020 • edited Loading

gnprice commented Apr 25, 2020

chrisbobbe commented Apr 25, 2020

gnprice commented May 1, 2020

chrisbobbe commented May 1, 2020

chrisbobbe Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented Jul 20, 2020 • edited Loading

chrisbobbe commented Apr 16, 2020 •

edited

Loading

chrisbobbe Apr 24, 2020 •

edited

Loading

chrisbobbe Apr 24, 2020 •

edited

Loading

chrisbobbe Apr 23, 2020 •

edited

Loading

chrisbobbe Apr 21, 2020 •

edited

Loading

chrisbobbe Apr 23, 2020 •

edited

Loading

gnprice commented Apr 22, 2020 •

edited

Loading

chrisbobbe commented Apr 22, 2020 •

edited

Loading

gnprice commented Apr 23, 2020 •

edited

Loading

chrisbobbe commented Apr 23, 2020 •

edited

Loading

chrisbobbe commented Apr 23, 2020 •

edited

Loading

chrisbobbe commented Apr 24, 2020 •

edited

Loading

chrisbobbe commented Apr 25, 2020 •

edited

Loading

chrisbobbe commented Apr 25, 2020 •

edited

Loading

chrisbobbe commented Apr 25, 2020 •

edited

Loading

chrisbobbe Jul 10, 2020 •

edited

Loading

chrisbobbe commented Jul 20, 2020 •

edited

Loading