Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace connect with useSelector in all our component definitions. #4837

Open
chrisbobbe opened this issue Jun 30, 2021 · 1 comment
Open
Assignees

Comments

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Jun 30, 2021

We've found another reason to do this: it'll help simplify the React Native v0.64 upgrade (#4426). We'll want to get closer to enabling a Flow feature called "Types-First", and add a few targeted suppressions where it turns out to be substantial work to satisfy Flow. See #4426 (comment) and following. See #4907.

Having tried Types-First experimentally, I get several dozen complaints that our exported component definitions that are made with connect aren't sufficiently annotated. The codemod Flow provides seems to stumble and not be able to fix these automatically. While we could do them by hand, it's probably cleanest and easiest just to get rid of the connect calls.

We've already gotten a good start on converting our components to function components. We should continue, with particular interest in the ones that use connect. There are 36 as of writing this.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jun 30, 2021
@chrisbobbe chrisbobbe self-assigned this Jun 30, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 1, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 1, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 1, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 1, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 1, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 1, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 1, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 1, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 1, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 2, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 2, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 2, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 2, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 2, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 2, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 2, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 2, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 2, 2021
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 2, 2021
@gnprice
Copy link
Member

gnprice commented Jul 2, 2021

Sounds good! There's one place where we know we need connect instead of useSelector because of the subtle differences in how they actually work; but I guess we've already tucked that inside withHaveServerDataGate, so its result isn't directly exported and doesn't affect this.

I've just merged #4838 and #4844, and it looks like git grep -E '\bconnect\(' is now down to 26 matches, from 36 (one of which is that connect in withHaveServerDataGate; the rest are indeed exported.)

There's also a number of places where we do the same thing except we provide type arguments to connect:

$ git grep -E '\bconnect<' src/ 
src/account-info/AccountDetails.js:export default connect<SelectorProps, _, _>((state, props) => ({
…

$ git grep -El '\bconnect<' src/  | wc -l
26

One of those is the type-wrapper src/react-redux.js itself; the other 25 are components, all in export default definitions except some that are exported after another step or two. Those include MessageList and ComposeBox, which may be among the most complex to convert to function components.

Do those cases also cause this sort of trouble, or do the type parameters make it OK?

In any case, if you run out of easy conversions and there are still a few left, we can always do annotations for those, even if the codemod is no help. For ComposeBox in particular, I expect it will be less work to write an annotation than to convert it to a function component. For one thing, the stakes are lower, because a mistake in the annotation isn't a live bug.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 7, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 7, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 7, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 7, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 7, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 7, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
An instance of zulip#4837.

Making sure to keep our chosen annotation for `stream`, which is
looser than `getStreamInNarrow`'s return type. See b21bf43 and
  zulip#4520 (comment).
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
An instance of zulip#4837.

Making sure to keep our chosen annotation for `stream`, which is
looser than `getStreamInNarrow`'s return type. See b21bf43 and
  zulip#4520 (comment).
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants