Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

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

Closed
wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0debad2
Remove legacy consumers of the SDKContext in favour of HOCs
t3chguy Apr 19, 2024
dc4c33a
Remove legacy consumers of the OverflowMenuContext in favour of HOCs
t3chguy Apr 19, 2024
f4296b4
Remove legacy consumers of the RovingTabIndexContext in favour of HOCs
t3chguy Apr 19, 2024
9fe036a
Remove legacy consumers of the RoomContext in favour of HOCs
t3chguy Apr 19, 2024
ffd8b7f
Remove legacy consumers of the MatrixClientContext in favour of HOCs
t3chguy Apr 19, 2024
4aaf049
Fix missed accessors
t3chguy Apr 19, 2024
6850ff6
Iterate
t3chguy Apr 19, 2024
c7db018
Iterate
t3chguy May 1, 2024
609dd52
Merge branch 'develop' of github.com:matrix-org/matrix-react-sdk into…
t3chguy May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions src/components/structures/EmbeddedPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ import { logger } from "matrix-js-sdk/src/logger";
import { _t, TranslationKey } from "../../languageHandler";
import dis from "../../dispatcher/dispatcher";
import { MatrixClientPeg } from "../../MatrixClientPeg";
import MatrixClientContext from "../../contexts/MatrixClientContext";
import { MatrixClientProps, withMatrixClientHOC } from "../../contexts/MatrixClientContext";
import AutoHideScrollbar from "./AutoHideScrollbar";
import { ActionPayload } from "../../dispatcher/payloads";

interface IProps {
interface IProps extends MatrixClientProps {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// URL to request embedded page content from
url?: string;
// Class name prefix to apply for a given instance
Expand All @@ -43,13 +43,12 @@ interface IState {
page: string;
}

export default class EmbeddedPage extends React.PureComponent<IProps, IState> {
public static contextType = MatrixClientContext;
class EmbeddedPage extends React.PureComponent<IProps, IState> {
private unmounted = false;
private dispatcherRef: string | null = null;

public constructor(props: IProps, context: typeof MatrixClientContext) {
super(props, context);
public constructor(props: IProps) {
super(props);

this.state = {
page: "",
Expand Down Expand Up @@ -120,7 +119,7 @@ export default class EmbeddedPage extends React.PureComponent<IProps, IState> {

public render(): React.ReactNode {
// HACK: Workaround for the context's MatrixClient not updating.
const client = this.context || MatrixClientPeg.get();
const client = this.props.mxClient || MatrixClientPeg.get();
const isGuest = client ? client.isGuest() : true;
const className = this.props.className;
const classes = classnames(className, {
Expand All @@ -137,3 +136,5 @@ export default class EmbeddedPage extends React.PureComponent<IProps, IState> {
}
}
}

export default withMatrixClientHOC(EmbeddedPage);
13 changes: 7 additions & 6 deletions src/components/structures/FilePanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import React, { createRef } from "react";
import React, { createRef, forwardRef } from "react";
import {
Filter,
EventTimelineSet,
Expand Down Expand Up @@ -45,6 +45,7 @@ interface IProps {
roomId: string;
onClose: () => void;
resizeNotifier: ResizeNotifier;
context: React.ContextType<typeof RoomContext>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

interface IState {
Expand All @@ -56,8 +57,6 @@ interface IState {
* Component which shows the filtered file using a TimelinePanel
*/
class FilePanel extends React.Component<IProps, IState> {
public static contextType = RoomContext;

// This is used to track if a decrypted event was a live event and should be
// added to the timeline.
private decryptingEvents = new Set<string>();
Expand Down Expand Up @@ -267,7 +266,7 @@ class FilePanel extends React.Component<IProps, IState> {
return (
<RoomContext.Provider
value={{
...this.context,
...this.props.context,
timelineRenderingType: TimelineRenderingType.File,
narrow: this.state.narrow,
}}
Expand Down Expand Up @@ -299,7 +298,7 @@ class FilePanel extends React.Component<IProps, IState> {
return (
<RoomContext.Provider
value={{
...this.context,
...this.props.context,
timelineRenderingType: TimelineRenderingType.File,
}}
>
Expand All @@ -312,4 +311,6 @@ class FilePanel extends React.Component<IProps, IState> {
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<RoomContext.Consumer>{(context) => <FilePanel {...props} context={context} ref={ref} />}</RoomContext.Consumer>
));
Comment on lines +314 to +316
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I find this quite hard to read. Splitting it up into separate lines makes it clearer, if Prettier allows it.

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

Similarly throughout.

3 changes: 1 addition & 2 deletions src/components/structures/LoggedInView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ import AudioFeedArrayForLegacyCall from "../views/voip/AudioFeedArrayForLegacyCa
import { OwnProfileStore } from "../../stores/OwnProfileStore";
import { UPDATE_EVENT } from "../../stores/AsyncStore";
import RoomView from "./RoomView";
import type { RoomView as RoomViewType } from "./RoomView";
import ToastContainer from "./ToastContainer";
import UserView from "./UserView";
import BackdropPanel from "./BackdropPanel";
Expand Down Expand Up @@ -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>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

protected readonly _resizeContainer: React.RefObject<HTMLDivElement>;
protected readonly resizeHandler: React.RefObject<HTMLDivElement>;
protected layoutWatcherRef?: string;
Expand Down
63 changes: 39 additions & 24 deletions src/components/structures/MessagePanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import React, { createRef, ReactNode, TransitionEvent } from "react";
import React, { createRef, forwardRef, ReactNode, TransitionEvent } from "react";
import ReactDOM from "react-dom";
import classNames from "classnames";
import { Room, MatrixClient, RoomStateEvent, EventStatus, MatrixEvent, EventType } from "matrix-js-sdk/src/matrix";
Expand All @@ -31,7 +31,6 @@ import EventTile, {
GetRelationsForEvent,
IReadReceiptProps,
isEligibleForSpecialReceipt,
UnwrappedEventTile,
} from "../views/rooms/EventTile";
import IRCTimelineProfileResizer from "../views/elements/IRCTimelineProfileResizer";
import defaultDispatcher from "../../dispatcher/dispatcher";
Expand Down Expand Up @@ -188,6 +187,7 @@ interface IProps {
disableGrouping?: boolean;

callEventGroupers: Map<string, LegacyCallEventGrouper>;
context: React.ContextType<typeof RoomContext>;
}

interface IState {
Expand All @@ -203,10 +203,7 @@ interface IReadReceiptForUser {

/* (almost) stateless UI component which builds the event tiles in the room timeline.
*/
export default class MessagePanel extends React.Component<IProps, IState> {
public static contextType = RoomContext;
public context!: React.ContextType<typeof RoomContext>;

class MessagePanel extends React.Component<IProps, IState> {
public static defaultProps = {
disableGrouping: false,
};
Expand Down Expand Up @@ -256,13 +253,13 @@ export default class MessagePanel extends React.Component<IProps, IState> {
private scrollPanel = createRef<ScrollPanel>();

private readonly showTypingNotificationsWatcherRef: string;
private eventTiles: Record<string, UnwrappedEventTile> = {};
private eventTiles: Record<string, React.ComponentRef<typeof EventTile>> = {};

// A map to allow groupers to maintain consistent keys even if their first event is uprooted due to back-pagination.
public grouperKeyMap = new WeakMap<MatrixEvent, string>();

public constructor(props: IProps, context: React.ContextType<typeof RoomContext>) {
super(props, context);
public constructor(props: IProps) {
super(props);

this.state = {
// previous positions the read marker has been in, so we can
Expand Down Expand Up @@ -320,7 +317,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
defaultDispatcher.dispatch({
action: Action.EditEvent,
event: !event?.isRedacted() ? event : null,
timelineRenderingType: this.context.timelineRenderingType,
timelineRenderingType: this.props.context.timelineRenderingType,
});
}
}
Expand Down Expand Up @@ -354,7 +351,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
return this.eventTiles[eventId]?.ref?.current ?? undefined;
}

public getTileForEventId(eventId?: string): UnwrappedEventTile | undefined {
public getTileForEventId(eventId?: string): React.ComponentRef<typeof EventTile> | undefined {
if (!this.eventTiles || !eventId) {
return undefined;
}
Expand Down Expand Up @@ -454,7 +451,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
};

public get showHiddenEvents(): boolean {
return this.context?.showHiddenEvents ?? this._showHiddenEvents;
return this.props.context?.showHiddenEvents ?? this._showHiddenEvents;
}

// TODO: Implement granular (per-room) hide options
Expand All @@ -481,7 +478,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
// Always show highlighted event
if (this.props.highlightedEventId === mxEv.getId()) return true;

return !shouldHideEvent(mxEv, this.context);
return !shouldHideEvent(mxEv, this.props.context);
}

public readMarkerForEvent(eventId: string, isLastEvent: boolean): ReactNode {
Expand Down Expand Up @@ -592,7 +589,9 @@ export default class MessagePanel extends React.Component<IProps, IState> {
}

try {
return localStorage.getItem(editorRoomKey(this.props.room.roomId, this.context.timelineRenderingType));
return localStorage.getItem(
editorRoomKey(this.props.room.roomId, this.props.context.timelineRenderingType),
);
} catch (err) {
logger.error(err);
return null;
Expand Down Expand Up @@ -775,13 +774,25 @@ export default class MessagePanel extends React.Component<IProps, IState> {
willWantSeparator === SeparatorKind.Date ||
mxEv.getSender() !== nextEv.getSender() ||
getEventDisplayInfo(cli, nextEv, this.showHiddenEvents).isInfoMessage ||
!shouldFormContinuation(mxEv, nextEv, cli, this.showHiddenEvents, this.context.timelineRenderingType);
!shouldFormContinuation(
mxEv,
nextEv,
cli,
this.showHiddenEvents,
this.props.context.timelineRenderingType,
);
}

// is this a continuation of the previous message?
const continuation =
wantsSeparator === SeparatorKind.None &&
shouldFormContinuation(prevEvent, mxEv, cli, this.showHiddenEvents, this.context.timelineRenderingType);
shouldFormContinuation(
prevEvent,
mxEv,
cli,
this.showHiddenEvents,
this.props.context.timelineRenderingType,
);

const eventId = mxEv.getId()!;
const highlight = eventId === this.props.highlightedEventId;
Expand Down Expand Up @@ -826,7 +837,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
}

public wantsSeparator(prevEvent: MatrixEvent | null, mxEvent: MatrixEvent): SeparatorKind {
if (this.context.timelineRenderingType === TimelineRenderingType.ThreadsList) {
if (this.props.context.timelineRenderingType === TimelineRenderingType.ThreadsList) {
return SeparatorKind.None;
}

Expand Down Expand Up @@ -863,13 +874,13 @@ export default class MessagePanel extends React.Component<IProps, IState> {
return null;
}

const receiptDestination = this.context.threadId ? room.getThread(this.context.threadId) : room;
const receiptDestination = this.props.context.threadId ? room.getThread(this.props.context.threadId) : room;

const receipts: IReadReceiptProps[] = [];

if (!receiptDestination) {
logger.debug(
"Discarding request, could not find the receiptDestination for event: " + this.context.threadId,
"Discarding request, could not find the receiptDestination for event: " + this.props.context.threadId,
);
return receipts;
}
Expand Down Expand Up @@ -950,7 +961,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
return receiptsByEvent;
}

private collectEventTile = (eventId: string, node: UnwrappedEventTile): void => {
private collectEventTile = (eventId: string, node: React.ComponentRef<typeof EventTile>): void => {
this.eventTiles[eventId] = node;
};

Expand Down Expand Up @@ -1033,7 +1044,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
if (
this.props.room &&
this.state.showTypingNotifications &&
this.context.timelineRenderingType === TimelineRenderingType.Room
this.props.context.timelineRenderingType === TimelineRenderingType.Room
) {
whoIsTyping = (
<WhoIsTypingTile
Expand All @@ -1053,7 +1064,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
}

const classes = classNames(this.props.className, {
mx_MessagePanel_narrow: this.context.narrow,
mx_MessagePanel_narrow: this.props.context.narrow,
});

return (
Expand All @@ -1079,10 +1090,14 @@ export default class MessagePanel extends React.Component<IProps, IState> {
}
}

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

/**
* 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* by a consumer from the event alone, so has to be done by the event list processing code earlier.
*/
export interface WrappedEvent {
Expand Down
15 changes: 10 additions & 5 deletions src/components/structures/NotificationPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import React from "react";
import React, { forwardRef } from "react";
import { logger } from "matrix-js-sdk/src/logger";

import { _t } from "../../languageHandler";
Expand All @@ -29,6 +29,7 @@ import Heading from "../views/typography/Heading";

interface IProps {
onClose(): void;
context: React.ContextType<typeof RoomContext>;
}

interface IState {
Expand All @@ -38,9 +39,7 @@ interface IState {
/*
* Component which shows the global notification list using a TimelinePanel
*/
export default class NotificationPanel extends React.PureComponent<IProps, IState> {
public static contextType = RoomContext;

class NotificationPanel extends React.PureComponent<IProps, IState> {
private card = React.createRef<HTMLDivElement>();

public constructor(props: IProps) {
Expand Down Expand Up @@ -86,7 +85,7 @@ export default class NotificationPanel extends React.PureComponent<IProps, IStat
return (
<RoomContext.Provider
value={{
...this.context,
...this.props.context,
timelineRenderingType: TimelineRenderingType.Notification,
narrow: this.state.narrow,
}}
Expand Down Expand Up @@ -114,3 +113,9 @@ export default class NotificationPanel extends React.PureComponent<IProps, IStat
);
}
}

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