Skip to content

Commit

Permalink
Enable react-compiler eslint to spot antipatterns (#28652)
Browse files Browse the repository at this point in the history
* Switch to React18 useId

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Enable react-compiler eslint

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Fix an easy one

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Disable in tests

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Fix usage of useRef as memoization

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Fix mutation of external values in hooks

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Make React compiler happy about some frankly non-issues

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Fix MapMock

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Revert MemberListViewModel.tsx changes and disable linter per line

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Make viewmodel compatible with react-compiler linter

- Remove searchQuery ref/state and instead pass this query to the
  loadMember function.
- Now we no longer need a separate search function

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Co-authored-by: R Midhun Suresh <hi@midhun.dev>
  • Loading branch information
t3chguy and MidhunSureshR authored Jan 16, 2025
1 parent e5ca795 commit ef1597f
Show file tree
Hide file tree
Showing 35 changed files with 146 additions and 136 deletions.
5 changes: 4 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
module.exports = {
plugins: ["matrix-org"],
plugins: ["matrix-org", "eslint-plugin-react-compiler"],
extends: ["plugin:matrix-org/babel", "plugin:matrix-org/react", "plugin:matrix-org/a11y"],
parserOptions: {
project: ["./tsconfig.json"],
Expand Down Expand Up @@ -170,6 +170,8 @@ module.exports = {
"jsx-a11y/role-supports-aria-props": "off",

"matrix-org/require-copyright-header": "error",

"react-compiler/react-compiler": "error",
},
overrides: [
{
Expand Down Expand Up @@ -262,6 +264,7 @@ module.exports = {

// These are fine in tests
"no-restricted-globals": "off",
"react-compiler/react-compiler": "off",
},
},
{
Expand Down
1 change: 1 addition & 0 deletions __mocks__/maplibre-gl.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class MockMap extends EventEmitter {
setCenter = jest.fn();
setStyle = jest.fn();
fitBounds = jest.fn();
remove = jest.fn();
}
const MockMapInstance = new MockMap();

Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@
"eslint-plugin-jsx-a11y": "^6.5.1",
"eslint-plugin-matrix-org": "^2.0.2",
"eslint-plugin-react": "^7.28.0",
"eslint-plugin-react-compiler": "^19.0.0-beta-df7b47d-20241124",
"eslint-plugin-react-hooks": "^5.0.0",
"eslint-plugin-unicorn": "^56.0.0",
"express": "^4.18.2",
Expand Down
1 change: 1 addition & 0 deletions src/accessibility/RovingTabIndex.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,7 @@ export const useRovingTabIndex = <T extends HTMLElement>(
});
}, []); // eslint-disable-line react-hooks/exhaustive-deps

// eslint-disable-next-line react-compiler/react-compiler
const isActive = context.state.activeNode === nodeRef.current;
return [onFocus, isActive, ref, nodeRef];
};
Expand Down
1 change: 1 addition & 0 deletions src/components/structures/AutocompleteInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ export const AutocompleteInput: React.FC<AutocompleteInputProps> = ({
{isFocused && suggestions.length ? (
<div
className="mx_AutocompleteInput_matches"
// eslint-disable-next-line react-compiler/react-compiler
style={{ top: editorContainerRef.current?.clientHeight }}
data-testid="autocomplete-matches"
>
Expand Down
1 change: 1 addition & 0 deletions src/components/structures/ContextMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,7 @@ export const useContextMenu = <T extends any = HTMLElement>(inputRef?: RefObject
setIsOpen(false);
};

// eslint-disable-next-line react-compiler/react-compiler
return [button.current ? isOpen : false, button, open, close, setIsOpen];
};

Expand Down
4 changes: 1 addition & 3 deletions src/components/structures/FilePanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -286,9 +286,7 @@ class FilePanel extends React.Component<IProps, IState> {
ref={this.card}
header={_t("right_panel|files_button")}
>
{this.card.current && (
<Measured sensor={this.card.current} onMeasurement={this.onMeasurement} />
)}
<Measured sensor={this.card} onMeasurement={this.onMeasurement} />
<SearchWarning isRoomEncrypted={isRoomEncrypted} kind={WarningKind.Files} />
<TimelinePanel
manageReadReceipts={false}
Expand Down
2 changes: 1 addition & 1 deletion src/components/structures/NotificationPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ export default class NotificationPanel extends React.PureComponent<IProps, IStat
onClose={this.props.onClose}
withoutScrollContainer={true}
>
{this.card.current && <Measured sensor={this.card.current} onMeasurement={this.onMeasurement} />}
<Measured sensor={this.card} onMeasurement={this.onMeasurement} />
{content}
</BaseCard>
</ScopedRoomContextProvider>
Expand Down
4 changes: 2 additions & 2 deletions src/components/structures/RoomSearchView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com
Please see LICENSE files in the repository root for full details.
*/

import React, { forwardRef, useCallback, useContext, useEffect, useRef, useState } from "react";
import React, { forwardRef, useCallback, useContext, useEffect, useMemo, useRef, useState } from "react";
import {
ISearchResults,
IThreadBundledRelationship,
Expand Down Expand Up @@ -58,7 +58,7 @@ export const RoomSearchView = forwardRef<ScrollPanel, Props>(
const [results, setResults] = useState<ISearchResults | null>(null);
const aborted = useRef(false);
// A map from room ID to permalink creator
const permalinkCreators = useRef(new Map<string, RoomPermalinkCreator>()).current;
const permalinkCreators = useMemo(() => new Map<string, RoomPermalinkCreator>(), []);
const innerRef = useRef<ScrollPanel | null>();

useEffect(() => {
Expand Down
5 changes: 2 additions & 3 deletions src/components/structures/RoomView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ function LocalRoomView(props: LocalRoomViewProps): ReactElement {
}

const onRetryClicked = (): void => {
// eslint-disable-next-line react-compiler/react-compiler
room.state = LocalRoomState.NEW;
defaultDispatcher.dispatch({
action: "local_room_event",
Expand Down Expand Up @@ -2514,9 +2515,7 @@ export class RoomView extends React.Component<IRoomProps, IRoomState> {
mainSplitContentClassName = "mx_MainSplit_timeline";
mainSplitBody = (
<>
{this.roomViewBody.current && (
<Measured sensor={this.roomViewBody.current} onMeasurement={this.onMeasurement} />
)}
<Measured sensor={this.roomViewBody} onMeasurement={this.onMeasurement} />
{auxPanel}
{pinnedMessageBanner}
<main className={timelineClasses}>
Expand Down
2 changes: 1 addition & 1 deletion src/components/structures/ThreadPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ const ThreadPanel: React.FC<IProps> = ({ roomId, onClose, permalinkCreator }) =>
ref={card}
closeButtonRef={closeButonRef}
>
{card.current && <Measured sensor={card.current} onMeasurement={setNarrow} />}
<Measured sensor={card} onMeasurement={setNarrow} />
{timelineSet ? (
<TimelinePanel
key={filterOption + ":" + (timelineSet.getFilter()?.filterId ?? roomId)}
Expand Down
2 changes: 1 addition & 1 deletion src/components/structures/ThreadView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ export default class ThreadView extends React.Component<IProps, IState> {
PosthogTrackers.trackInteraction("WebThreadViewBackButton", ev);
}}
>
{this.card.current && <Measured sensor={this.card.current} onMeasurement={this.onMeasurement} />}
<Measured sensor={this.card} onMeasurement={this.onMeasurement} />
<div className="mx_ThreadView_timelinePanelWrapper">{timeline}</div>

{ContentMessages.sharedInstance().getCurrentUploads(threadRelation).length > 0 && (
Expand Down
35 changes: 8 additions & 27 deletions src/components/utils/Box.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ Please see LICENSE files in the repository root for full details.
*/

import classNames from "classnames";
import React, { useEffect, useRef } from "react";
import React, { useMemo } from "react";

type FlexProps = {
/**
Expand Down Expand Up @@ -40,25 +40,6 @@ type FlexProps = {
grow?: string | null;
};

/**
* Set or remove a CSS property
* @param ref the reference
* @param name the CSS property name
* @param value the CSS property value
*/
function addOrRemoveProperty(
ref: React.MutableRefObject<HTMLElement | undefined>,
name: string,
value?: string | null,
): void {
const style = ref.current!.style;
if (value) {
style.setProperty(name, value);
} else {
style.removeProperty(name);
}
}

/**
* A flex child helper
*/
Expand All @@ -71,12 +52,12 @@ export function Box({
children,
...props
}: React.PropsWithChildren<FlexProps>): JSX.Element {
const ref = useRef<HTMLElement>();

useEffect(() => {
addOrRemoveProperty(ref, `--mx-box-flex`, flex);
addOrRemoveProperty(ref, `--mx-box-shrink`, shrink);
addOrRemoveProperty(ref, `--mx-box-grow`, grow);
const style = useMemo(() => {
const style: Record<string, any> = {};
if (flex) style["--mx-box-flex"] = flex;
if (shrink) style["--mx-box-shrink"] = shrink;
if (grow) style["--mx-box-grow"] = grow;
return style;
}, [flex, grow, shrink]);

return React.createElement(
Expand All @@ -88,7 +69,7 @@ export function Box({
"mx_Box--shrink": !!shrink,
"mx_Box--grow": !!grow,
}),
ref,
style,
},
children,
);
Expand Down
23 changes: 12 additions & 11 deletions src/components/utils/Flex.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ Please see LICENSE files in the repository root for full details.
*/

import classNames from "classnames";
import React, { useEffect, useRef } from "react";
import React, { useMemo } from "react";

type FlexProps = {
/**
Expand Down Expand Up @@ -64,15 +64,16 @@ export function Flex({
children,
...props
}: React.PropsWithChildren<FlexProps>): JSX.Element {
const ref = useRef<HTMLElement>();
const style = useMemo(
() => ({
"--mx-flex-display": display,
"--mx-flex-direction": direction,
"--mx-flex-align": align,
"--mx-flex-justify": justify,
"--mx-flex-gap": gap,
}),
[align, direction, display, gap, justify],
);

useEffect(() => {
ref.current!.style.setProperty(`--mx-flex-display`, display);
ref.current!.style.setProperty(`--mx-flex-direction`, direction);
ref.current!.style.setProperty(`--mx-flex-align`, align);
ref.current!.style.setProperty(`--mx-flex-justify`, justify);
ref.current!.style.setProperty(`--mx-flex-gap`, gap);
}, [align, direction, display, gap, justify]);

return React.createElement(as, { ...props, className: classNames("mx_Flex", className), ref }, children);
return React.createElement(as, { ...props, className: classNames("mx_Flex", className), style }, children);
}
31 changes: 10 additions & 21 deletions src/components/viewmodels/memberlist/MemberListViewModel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
UserEvent,
} from "matrix-js-sdk/src/matrix";
import { KnownMembership } from "matrix-js-sdk/src/types";
import { useCallback, useContext, useEffect, useMemo, useRef, useState } from "react";
import { useCallback, useContext, useEffect, useMemo, useState } from "react";
import { throttle } from "lodash";

import { RoomMember } from "../../../models/rooms/RoomMember";
Expand Down Expand Up @@ -120,19 +120,16 @@ export function useMemberListViewModel(roomId: string): MemberListViewState {
const sdkContext = useContext(SDKContext);
const [memberMap, setMemberMap] = useState<Map<string, Member>>(new Map());
const [isLoading, setIsLoading] = useState<boolean>(true);

// This is the last known total number of members in this room.
const totalMemberCount = useRef<number>(0);

const searchQuery = useRef("");
const [totalMemberCount, setTotalMemberCount] = useState(0);

const loadMembers = useMemo(
() =>
throttle(
async (): Promise<void> => {
async (searchQuery?: string): Promise<void> => {
const { joined: joinedSdk, invited: invitedSdk } = await sdkContext.memberListStore.loadMemberList(
roomId,
searchQuery.current,
searchQuery,
);
const newMemberMap = new Map<string, Member>();
// First add the invited room members
Expand All @@ -141,7 +138,7 @@ export function useMemberListViewModel(roomId: string): MemberListViewState {
newMemberMap.set(member.userId, roomMember);
}
// Then add the third party invites
const threePidInvited = getPending3PidInvites(room, searchQuery.current);
const threePidInvited = getPending3PidInvites(room, searchQuery);
for (const invited of threePidInvited) {
const key = invited.threePidInvite!.event.getContent().display_name;
newMemberMap.set(key, invited);
Expand All @@ -152,26 +149,18 @@ export function useMemberListViewModel(roomId: string): MemberListViewState {
newMemberMap.set(member.userId, roomMember);
}
setMemberMap(newMemberMap);
if (!searchQuery.current) {
if (!searchQuery) {
/**
* Since searching for members only gives you the relevant
* members matching the query, do not update the totalMemberCount!
**/
totalMemberCount.current = newMemberMap.size;
setTotalMemberCount(newMemberMap.size);
}
},
500,
{ leading: true, trailing: true },
),
[roomId, sdkContext.memberListStore, room],
);

const search = useCallback(
(query: string) => {
searchQuery.current = query;
loadMembers();
},
[loadMembers],
[sdkContext.memberListStore, roomId, room],
);

const isPresenceEnabled = useMemo(
Expand Down Expand Up @@ -252,12 +241,12 @@ export function useMemberListViewModel(roomId: string): MemberListViewState {

return {
members: Array.from(memberMap.values()),
search,
search: loadMembers,
shouldShowInvite,
isPresenceEnabled,
isLoading,
onInviteButtonClick,
shouldShowSearch: totalMemberCount.current >= 20,
shouldShowSearch: totalMemberCount >= 20,
canInvite,
};
}
3 changes: 1 addition & 2 deletions src/components/views/elements/EffectsOverlay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,10 @@ const EffectsOverlay: FunctionComponent<IProps> = ({ roomWidth }) => {
if (canvas) canvas.height = UIStore.instance.windowHeight;
UIStore.instance.on(UI_EVENTS.Resize, resize);

const currentEffects = effectsRef.current; // this is not a react node ref, warning can be safely ignored
return () => {
dis.unregister(dispatcherRef);
UIStore.instance.off(UI_EVENTS.Resize, resize);
// eslint-disable-next-line react-hooks/exhaustive-deps
const currentEffects = effectsRef.current; // this is not a react node ref, warning can be safely ignored
for (const effect in currentEffects) {
const effectModule: ICanvasEffect = currentEffects.get(effect)!;
if (effectModule && effectModule.isRunning) {
Expand Down
10 changes: 5 additions & 5 deletions src/components/views/elements/Measured.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com
Please see LICENSE files in the repository root for full details.
*/

import React from "react";
import React, { RefObject } from "react";

import UIStore, { UI_EVENTS } from "../../../stores/UIStore";

interface IProps {
sensor: Element;
sensor: RefObject<Element>;
breakpoint: number;
onMeasurement(narrow: boolean): void;
}
Expand All @@ -35,14 +35,14 @@ export default class Measured extends React.PureComponent<IProps> {
}

public componentDidUpdate(prevProps: Readonly<IProps>): void {
const previous = prevProps.sensor;
const current = this.props.sensor;
const previous = prevProps.sensor.current;
const current = this.props.sensor.current;
if (previous === current) return;
if (previous) {
UIStore.instance.stopTrackingElementDimensions(`Measured${this.instanceId}`);
}
if (current) {
UIStore.instance.trackElementDimensions(`Measured${this.instanceId}`, this.props.sensor);
UIStore.instance.trackElementDimensions(`Measured${this.instanceId}`, this.props.sensor.current);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/components/views/right_panel/TimelineCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ export default class TimelineCard extends React.Component<IProps, IState> {
header={_t("right_panel|video_room_chat|title")}
ref={this.card}
>
{this.card.current && <Measured sensor={this.card.current} onMeasurement={this.onMeasurement} />}
<Measured sensor={this.card} onMeasurement={this.onMeasurement} />
<div className="mx_TimelineCard_timeline">
{jumpToBottom}
<TimelinePanel
Expand Down
1 change: 1 addition & 0 deletions src/components/views/rooms/ReadReceiptGroup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ export function ReadReceiptGroup({
readReceiptPosition = readReceiptMap[userId];
if (!readReceiptPosition) {
readReceiptPosition = {};
// eslint-disable-next-line react-compiler/react-compiler
readReceiptMap[userId] = readReceiptPosition;
}
}
Expand Down
Loading

0 comments on commit ef1597f

Please sign in to comment.