Remove legacy consumers of the React contexts in favour of HOCs

[t3chguy]

For https://github.com/element-hq/wat-internal/issues/65 Closes https://github.com/matrix-org/matrix-react-sdk/pull/10311

[richvdh]

For https://github.com/element-hq/wat-internal/issues/65

For external readers, this is "Upgrade to react 18"

[richvdh]

More questions:

• How does this relate to https://github.com/matrix-org/matrix-react-sdk/pull/10311?
• How does this relate to MatrixClientContextProvider ?

[richvdh]

How does this relate to MatrixClientContextProvider ?

Oh, stupid question, I think. MatrixClientContextProvider is about things that provide the context, while the HOC thing is about things that receive it. I think? Documentation would sure be nice.

[t3chguy]

WTF is a HOC

https://legacy.reactjs.org/docs/higher-order-components.html

and why is it better than a Context object?

It is a Context object, just passed like a normal prop with normal types rather than the mess that is

    public static contextType = RoomContext;
    public context!: React.ContextType<typeof RoomContext>;

• Which doesn't set context until after the constructor finishes
• Which messes up the types significantly
• Which only supports consuming one context, so you can't have Room + MatrixClient for example.

How does this relate to https://github.com/matrix-org/matrix-react-sdk/pull/10311?

It is an alternative path towards React 18 which isn't blocked on a decision between Typescript & Babel to land.

How does this relate to MatrixClientContextProvider ?

This is a manner of consuming a context, like useContext hook is for FCs. XYZContext.Provider is a way of providing a context to be consumed by distant children.

If we're getting rid of MatrixClientContext, we should probably update the documentation on it to say that it's deprecated.

We're not. We're just not going to consume it via the really limited class component context field due to the limitations outlined in bullets above

Can we do this in a way that doesn't involve changing and reviewing 96 files at once? Sorry, but it's extremely unlikely I'm going to have time to review this as it stands.

We can split it into the constituent commits, are they not good enough here?

[richvdh]

WTF is a HOC

https://legacy.reactjs.org/docs/higher-order-components.html

... which says: Higher-order components are not commonly used in modern React code. But that's by the by.

Anyway, please could the HOC types be documented rather than relying on institutional knowledge?

If we're getting rid of MatrixClientContext, we should probably update the documentation on it to say that it's deprecated.

We're not. We're just not going to consume it via the really limited class component context field due to the limitations outlined in bullets above

Ok, fair enough, but probably we could helpfully write in MatrixClientContext's documentation that you probably want a withMatrixClientHoc rater than directly referring to MatrixClientContext?

We can split it into the constituent commits, are they not good enough here?

I'm afraid not, really. I don't want to have to plough through this massive PR, whether it's broken into commits or not.

[t3chguy]

Ok, fair enough, but probably we could helpfully write in MatrixClientContext's documentation that you probably want a withMatrixClientHoc rater than directly referring to MatrixClientContext?

There's no reason to use withMatrixClientHoc over a custom HOC or just MatrixClientContext.Consumer - its just a bit less boilerplatey by means of consolidation

[richvdh]

There's no reason to use withMatrixClientHoc over a custom HOC or just MatrixClientContext.Consumer - its just a bit less boilerplatey by means of consolidation

Please, write that down in the documentation then.

[richvdh]

I'm sorry that I've been sitting on this for a week, but also:

This makes it really hard to fit a review in around other work. I'm much more likely to be able to do smaller reviews promptly; a review of this size, changing this many different things, requires several hours of uninterrupted time to review properly.

Part of the blame is on me here for not rejecting it sooner, but I really feel like it's the author's job to make things easy for the reviewer without having to be asked; it shouldn't be on the reviewer to have to decide where to draw a line.

Please do try to make PRs smaller.

[t3chguy]

Thanks for the review. Don't have that much time to spend on this anymore. Will close it until someone from the wysiwyg or compound teams can resume it for their benefit.