Remove legacy consumers of the React contexts in favour of HOCs #12444

t3chguy · 2024-04-19T14:49:11Z

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

Signed-off-by: Michael Telatynski <[email protected]>

richvdh · 2024-04-22T12:18:35Z

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

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

richvdh

So many questions.

WTF is a HOC, and why is it better than a Context object? (There doesn't seem to be any documentation at all on matrixHOC or withMatrixClientHOC; I'd expect that to be a pre-requisite for this sort of large-scale migration).
If we're getting rid of MatrixClientContext, we should probably update the documentation on it to say that it's deprecated.
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.

richvdh · 2024-04-22T12:27:50Z

More questions:

How does this relate to Correctly specify types for modern React contexts #10311?
How does this relate to MatrixClientContextProvider ?

richvdh · 2024-04-22T12:29:33Z

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 · 2024-04-22T13:20:50Z

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 #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 · 2024-04-22T14:33:52Z

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 · 2024-04-22T14:35:58Z

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 · 2024-04-22T14:39:50Z

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.

Signed-off-by: Michael Telatynski <[email protected]>

… t3chguy/react18/context-hocs

richvdh · 2024-05-09T16:19:33Z

src/contexts/MatrixClientContext.tsx

+ * A higher order component that injects the MatrixClient into props.mxClient of the wrapped component.
+ * Preferred over using `static contextType` as the types for this are quite broken in React 17.
+ * Inherently no different to wrapping in MatrixClientContext.Consumer but saves a lot of boilerplate.
+ * @param ComposedComponent the ComponentClass you wish to wrap in the HOC
+ * @returns a new component that takes the same props as the original component, but with an additional mxClient prop


Suggested change

* A higher order component that injects the MatrixClient into props.mxClient of the wrapped component.

* Preferred over using `static contextType` as the types for this are quite broken in React 17.

* Inherently no different to wrapping in MatrixClientContext.Consumer but saves a lot of boilerplate.

* @param ComposedComponent the ComponentClass you wish to wrap in the HOC

* @returns a new component that takes the same props as the original component, but with an additional mxClient prop

* A higher order component that injects the `MatrixClient` into `props.mxClient` of the wrapped component.

*

* Preferred over using a [static `contextType` property](https://react.dev/reference/react/Component#static-contexttype)

* as the types for that are quite broken in React 17.

* Inherently, no different to wrapping in `MatrixClientContext.Consumer` but saves a lot of boilerplate.

*

* @param ComposedComponent - the `ComponentClass` you wish to wrap in the HOC

* @returns a new component class that takes the same props as the original component, but with an additional `mxClient` prop

takes the same props as the original component, but with an additional mxClient prop

This doesn't sound right. The original prop must take an mxClient prop, which we will populate from the context. The returned component omits the mxClient prop.

richvdh · 2024-05-09T16:22:32Z

src/components/structures/EmbeddedPage.tsx

 import AutoHideScrollbar from "./AutoHideScrollbar";
 import { ActionPayload } from "../../dispatcher/payloads";

-interface IProps {
+interface IProps extends MatrixClientProps {


honestly, given MatrixClientProps contains exactly one entry, I'd find it much clearer to spell it out explicitly, and it's hard to see much disadvantage to doing so. (We can't exactly just rename it in MatrixClientProps, because we rely on it being called mxClient` below.

Not a strong opinion though, and maybe not worth changing now.

richvdh · 2024-05-09T16:34:19Z

src/components/structures/LoggedInView.tsx

@@ -132,7 +131,7 @@ class LoggedInView extends React.Component<IProps, IState> {
    public static displayName = "LoggedInView";

    protected readonly _matrixClient: MatrixClient;
-    protected readonly _roomView: React.RefObject<RoomViewType>;
+    protected readonly _roomView: React.RefObject<React.ComponentRef<typeof RoomView>>;


I couldn't find any documentation for React.ComponentRef. Can you explain what it does, and why this change is needed?

richvdh · 2024-05-09T16:39:37Z

src/components/structures/MessagePanel.tsx

 /**
- * Holds on to an event, caching the information about it in the context of the current messages list.
+ * Holds on to an event, caching the information about it in the props.context. of the current messages list.


I'm far from convinced this is accurate, but let's not make it more misleading.

Suggested change

* Holds on to an event, caching the information about it in the props.context. of the current messages list.

* Holds on to an event, caching the information about it in the context of the current messages list.

richvdh · 2024-05-09T16:40:08Z

src/components/structures/MessagePanel.tsx

 * Avoids calling shouldShowEvent more times than we need to.
- * Simplifies threading of event context like whether it's the last successful event we sent which cannot be determined
+ * Simplifies threading of event props.context. like whether it's the last successful event we sent which cannot be determined


I think you've been too gung-ho with search and replace.

Suggested change

* Simplifies threading of event props.context. like whether it's the last successful event we sent which cannot be determined

* Simplifies threading of event context like whether it's the last successful event we sent which cannot be determined

richvdh · 2024-05-09T18:22:15Z

src/components/structures/FilePanel.tsx

@@ -45,6 +45,7 @@ interface IProps {
    roomId: string;
    onClose: () => void;
    resizeNotifier: ResizeNotifier;
+    context: React.ContextType<typeof RoomContext>;


I've been dithering about whether I want you to document these, and I've decided that I do, if only to tell callers that they don't need to worry about them.

You could argue that FilePanel (as defined on line 58, not as exported by FilePanel.ts 🤯) is not a public component, and therefore is not subject to the rules of the coding style doc that say "properties must be documented", but I think that is not in keeping with the spirit of the coding style doc. The point is that if I want to use a FilePanel, this is going to be my reference to the properties I need to provide, and I don't want to have to reverse-engineer the forwardRef incantation to realise that I don't need to specify a context here.

Suggested change

context: React.ContextType<typeof RoomContext>;

/** Details of the room in which this file belongs. Populated automatically from the `RoomContext`. */

context: React.ContextType<typeof RoomContext>;

Likewise throughout.

This also ties into making the mxClient properties explicit rather than inheriting from MatrixClientProps: it gives a place to document the fact that callers don't need to populate mxClient provided there is a MatrixClientContext.

richvdh · 2024-05-09T18:23:43Z

src/components/structures/FilePanel.tsx

@@ -312,4 +311,6 @@ class FilePanel extends React.Component<IProps, IState> {
    }
 }

-export default FilePanel;
+export default forwardRef<FilePanel, Omit<IProps, "context">>((props, ref) => (


Likewise, a word of documentation on this would be useful.

Suggested change

export default forwardRef<FilePanel, Omit<IProps, "context">>((props, ref) => (

/** Wraps a `FilePanel`, populating its `context` property from the current {@link RoomContext}. */

export default forwardRef<FilePanel, Omit<IProps, "context">>((props, ref) => (

Likewise throughout.

richvdh · 2024-05-09T18:29:33Z

src/components/views/rooms/BasicMessageComposer.tsx

@@ -123,7 +123,7 @@ interface IState {

 export default class BasicMessageEditor extends React.Component<IProps, IState> {
    public readonly editorRef = createRef<HTMLDivElement>();
-    private autocompleteRef = createRef<Autocomplete>();
+    private autocompleteRef = createRef<React.ComponentRef<typeof Autocomplete>>();


what's the difference here?

richvdh · 2024-05-09T18:33:21Z

src/components/views/rooms/EditMessageComposer.tsx

@@ -122,28 +122,25 @@ export function createEditContent(
 interface IEditMessageComposerProps extends MatrixClientProps {
    editState: EditorStateTransfer;
    className?: string;
+    context: React.ContextType<typeof RoomContext>;


got to say, calling all these things just context isn't the clearest thing, especially in cases like this where there are actually two contexts in play. roomContext would be a better name.

Not a showstopper though.

richvdh · 2024-05-09T18:41:56Z

src/components/views/rooms/ReplyPreview.tsx

+export default forwardRef<ReplyPreview, Omit<IProps, "context">>((props, ref) => (
+    <RoomContext.Consumer>{(context) => <ReplyPreview {...props} context={context} ref={ref} />}</RoomContext.Consumer>
+));


How come MatrixClientContext qualifies for a HOC to do this boilerplate, but the 15000 <RoomContext.Consumer>s don't, by the way?

richvdh · 2024-05-09T18:53:27Z

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 · 2024-05-09T20:02:17Z

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.

t3chguy added 5 commits April 19, 2024 12:38

Remove legacy consumers of the SDKContext in favour of HOCs

0debad2

Signed-off-by: Michael Telatynski <[email protected]>

Remove legacy consumers of the OverflowMenuContext in favour of HOCs

dc4c33a

Signed-off-by: Michael Telatynski <[email protected]>

Remove legacy consumers of the RovingTabIndexContext in favour of HOCs

f4296b4

Signed-off-by: Michael Telatynski <[email protected]>

Remove legacy consumers of the RoomContext in favour of HOCs

9fe036a

Signed-off-by: Michael Telatynski <[email protected]>

Remove legacy consumers of the MatrixClientContext in favour of HOCs

ffd8b7f

Signed-off-by: Michael Telatynski <[email protected]>

t3chguy added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Apr 19, 2024

t3chguy self-assigned this Apr 19, 2024

github-actions bot deployed to Netlify April 19, 2024 14:52 View deployment

Fix missed accessors

4aaf049

Signed-off-by: Michael Telatynski <[email protected]>

github-actions bot deployed to Netlify April 19, 2024 15:08 View deployment

Iterate

6850ff6

Signed-off-by: Michael Telatynski <[email protected]>

github-actions bot deployed to EndToEndTests April 19, 2024 15:30 View deployment

github-actions bot deployed to Netlify April 19, 2024 21:06 View deployment

github-actions bot deployed to EndToEndTests April 19, 2024 21:24 View deployment

t3chguy marked this pull request as ready for review April 22, 2024 08:10

t3chguy requested a review from a team as a code owner April 22, 2024 08:10

t3chguy requested review from richvdh and MidhunSureshR April 22, 2024 08:10

richvdh suggested changes Apr 22, 2024

View reviewed changes

t3chguy added 2 commits May 1, 2024 15:22

Iterate

c7db018

Signed-off-by: Michael Telatynski <[email protected]>

Merge branch 'develop' of github.com:matrix-org/matrix-react-sdk into…

609dd52

… t3chguy/react18/context-hocs

github-actions bot deployed to Netlify May 1, 2024 14:44 View deployment

github-actions bot deployed to EndToEndTests May 1, 2024 14:45 View deployment

t3chguy requested a review from richvdh May 1, 2024 14:50

richvdh suggested changes May 9, 2024

View reviewed changes

t3chguy closed this May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove legacy consumers of the React contexts in favour of HOCs #12444

Remove legacy consumers of the React contexts in favour of HOCs #12444

t3chguy commented Apr 19, 2024 •

edited

Loading

richvdh commented Apr 22, 2024

richvdh left a comment

richvdh commented Apr 22, 2024

richvdh commented Apr 22, 2024

t3chguy commented Apr 22, 2024 •

edited

Loading

richvdh commented Apr 22, 2024

t3chguy commented Apr 22, 2024 •

edited

Loading

richvdh commented Apr 22, 2024

richvdh May 9, 2024

richvdh May 9, 2024

richvdh May 9, 2024

richvdh May 9, 2024

richvdh May 9, 2024

richvdh May 9, 2024

richvdh May 9, 2024

richvdh May 9, 2024

richvdh May 9, 2024

richvdh May 9, 2024

richvdh May 9, 2024

richvdh commented May 9, 2024

t3chguy commented May 9, 2024 •

edited

Loading

- * A higher order component that injects the MatrixClient into props.mxClient of the wrapped component.
- * Preferred over using `static contextType` as the types for this are quite broken in React 17.
- * Inherently no different to wrapping in MatrixClientContext.Consumer but saves a lot of boilerplate.
- * @param ComposedComponent the ComponentClass you wish to wrap in the HOC
- * @returns a new component that takes the same props as the original component, but with an additional mxClient prop
+ * A higher order component that injects the `MatrixClient` into `props.mxClient` of the wrapped component.
+ *
+ * Preferred over using a [static `contextType` property](https://react.dev/reference/react/Component#static-contexttype)
+ * as the types for that are quite broken in React 17.
+ * Inherently, no different to wrapping in `MatrixClientContext.Consumer` but saves a lot of boilerplate.
+ *
+ * @param ComposedComponent - the `ComponentClass` you wish to wrap in the HOC
+ * @returns a new component class that takes the same props as the original component, but with an additional `mxClient` prop

	* Holds on to an event, caching the information about it in the props.context. of the current messages list.
	* Holds on to an event, caching the information about it in the context of the current messages list.

	* Simplifies threading of event props.context. like whether it's the last successful event we sent which cannot be determined
	* Simplifies threading of event context like whether it's the last successful event we sent which cannot be determined

	export default forwardRef<FilePanel, Omit<IProps, "context">>((props, ref) => (
	/** Wraps a `FilePanel`, populating its `context` property from the current {@link RoomContext}. */
	export default forwardRef<FilePanel, Omit<IProps, "context">>((props, ref) => (

Remove legacy consumers of the React contexts in favour of HOCs #12444

Remove legacy consumers of the React contexts in favour of HOCs #12444

Conversation

t3chguy commented Apr 19, 2024 • edited Loading

richvdh commented Apr 22, 2024

richvdh left a comment

Choose a reason for hiding this comment

richvdh commented Apr 22, 2024

richvdh commented Apr 22, 2024

t3chguy commented Apr 22, 2024 • edited Loading

richvdh commented Apr 22, 2024

t3chguy commented Apr 22, 2024 • edited Loading

richvdh commented Apr 22, 2024

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

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

richvdh commented May 9, 2024

t3chguy commented May 9, 2024 • edited Loading

t3chguy commented Apr 19, 2024 •

edited

Loading

t3chguy commented Apr 22, 2024 •

edited

Loading

t3chguy commented Apr 22, 2024 •

edited

Loading

t3chguy commented May 9, 2024 •

edited

Loading