diff --git a/.oxlintrc.json b/.oxlintrc.json index 2ec28ddc..5e10cd94 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -14,6 +14,7 @@ "browser": true }, "rules": { + "typescript/no-explicit-any": "error", "typescript/explicit-function-return-type": "error", "typescript/explicit-module-boundary-types": "error", "typescript/array-type": ["error", { "default": "array-simple" }], @@ -44,9 +45,18 @@ "react/jsx-no-useless-fragment": "warn", "react/no-unescaped-entities": "error", "react/exhaustive-deps": "error", - "react/react-compiler": "warn", - "react-doctor/no-fetch-in-effect": "warn", - "react-doctor/no-derived-state": "warn", + "react/unsupported-syntax": "error", + "react/no-deriving-state-in-effects": "error", + "react/invariant": "error", + "react/rule-suppression": "error", + "react/syntax": "error", + "react/todo": "error", + "react/capitalized-calls": "error", + "react/exhaustive-effect-dependencies": "error", + "react/hooks": "error", + "react/memo-dependencies": "error", + "react-doctor/no-fetch-in-effect": "error", + "react-doctor/no-derived-state": "error", "react/jsx-curly-brace-presence": [ "error", { "props": "never", "children": "never", "propElementValues": "always" } diff --git a/README.md b/README.md index 4ccbb3e2..da4aaaa8 100644 --- a/README.md +++ b/README.md @@ -64,8 +64,10 @@ npm run lint npm run lint:fix ``` -The Oxlint configuration includes React Doctor's lint-native rules. Run `npm run doctor` for the full React Doctor -project scan; pull requests and pushes to `main` also run that scan in GitHub Actions. +The Oxlint configuration treats React Compiler rules as errors, including the recommended correctness rules and the +compiler checks in the suspicious, performance, and restriction categories. React Doctor's `no-fetch-in-effect` and +`no-derived-state` rules are also errors. Run `npm run doctor` for the full React Doctor project scan; pull requests and +pushes to `main` also run that scan in GitHub Actions. #### Development mode diff --git a/STYLEGUIDE.md b/STYLEGUIDE.md index 8600e18d..4d0320fe 100644 --- a/STYLEGUIDE.md +++ b/STYLEGUIDE.md @@ -37,7 +37,7 @@ touch without expanding the task into unrelated cleanup. for dynamic values passed across memoized boundaries. Do not add memoization blindly or omit dependencies. - Memoize React context values when they contain objects or functions whose identity would otherwise change each render. - Avoid array-index keys when a stable domain identifier exists. -- Treat `react/react-compiler` and `react-perf` diagnostics as design feedback. Fix new warnings rather than disabling +- Treat React Compiler and `react-perf` diagnostics as design feedback. Fix new warnings rather than disabling the rule or increasing the repository warning ceiling. ## Components, state, and accessibility diff --git a/client/src/context/FetchContext.test.tsx b/client/src/context/FetchContext.test.tsx index ed84157f..a661effb 100644 --- a/client/src/context/FetchContext.test.tsx +++ b/client/src/context/FetchContext.test.tsx @@ -11,7 +11,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -import { render, screen, waitFor } from '@testing-library/react'; +import { render, renderHook, screen, waitFor } from '@testing-library/react'; import type { FetchFn } from './FetchContext'; import { FetchProvider, useFetch } from './FetchContext'; @@ -38,15 +38,9 @@ function TestJsonConsumer({ url }: { url: string }): React.ReactElement { describe('FetchContext', () => { describe('useFetch without provider', () => { it('returns the default fetch wrapper from @perses-dev/client', () => { - let hookResult: ReturnType | undefined; - function Capture(): React.ReactNode { - hookResult = useFetch(); - return null; - } - render(); - expect(hookResult).toBeDefined(); - expect(typeof hookResult!.fetch).toBe('function'); - expect(typeof hookResult!.fetchJson).toBe('function'); + const { result } = renderHook(() => useFetch()); + expect(typeof result.current.fetch).toBe('function'); + expect(typeof result.current.fetchJson).toBe('function'); }); }); diff --git a/client/src/model/resource.ts b/client/src/model/resource.ts index e5c193f6..187c897b 100644 --- a/client/src/model/resource.ts +++ b/client/src/model/resource.ts @@ -34,8 +34,7 @@ export interface ProjectMetadata extends Metadata { export interface Resource { kind: Kind; metadata: Metadata | ProjectMetadata; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - spec?: any; + spec?: unknown; } export interface ProjectResource { diff --git a/components/src/EChart/EChart.test.tsx b/components/src/EChart/EChart.test.tsx new file mode 100644 index 00000000..82d8cd5d --- /dev/null +++ b/components/src/EChart/EChart.test.tsx @@ -0,0 +1,58 @@ +// Copyright The Perses Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import { act, render } from '@testing-library/react'; +import { init } from 'echarts/core'; + +import { EChart } from './EChart'; + +const option = {}; + +it('resizes on container changes and cancels pending work when unmounted', () => { + vi.useFakeTimers(); + const resize = vi.fn(); + const dispose = vi.fn(); + const observe = vi.fn(); + const disconnect = vi.fn(); + const callbacks: ResizeObserverCallback[] = []; + class TestResizeObserver { + observe = observe; + disconnect = disconnect; + unobserve = vi.fn(); + constructor(callback: ResizeObserverCallback) { + callbacks.push(callback); + } + } + vi.stubGlobal('ResizeObserver', TestResizeObserver); + vi.mocked(init).mockReturnValue({ setOption: vi.fn(), resize, dispose } as unknown as ReturnType); + try { + const { unmount } = render(); + expect(observe).toHaveBeenCalledTimes(1); + act(() => vi.advanceTimersByTime(200)); + expect(resize).toHaveBeenCalledTimes(1); + act(() => { + callbacks[0]?.([], {} as ResizeObserver); + vi.advanceTimersByTime(200); + }); + expect(resize).toHaveBeenCalledTimes(2); + act(() => callbacks[0]?.([], {} as ResizeObserver)); + unmount(); + act(() => vi.runAllTimers()); + expect(disconnect).toHaveBeenCalledTimes(1); + expect(dispose).toHaveBeenCalledTimes(1); + expect(resize).toHaveBeenCalledTimes(2); + } finally { + vi.unstubAllGlobals(); + vi.useRealTimers(); + } +}); diff --git a/components/src/EChart/EChart.tsx b/components/src/EChart/EChart.tsx index b00caf70..01b91992 100644 --- a/components/src/EChart/EChart.tsx +++ b/components/src/EChart/EChart.tsx @@ -169,7 +169,7 @@ export const EChart = memo(function EChart({ sx, style, onEvents, - _instance, + _instance: instanceRef, syncGroup, onChartInitialized, }: EChartsProps) { @@ -185,15 +185,15 @@ export const EChart = memo(function EChart({ if (chartElement.current === undefined) return; chartElement.current.setOption(initialOption.current, true); onChartInitialized?.(chartElement.current); - if (_instance !== undefined) { - _instance.current = chartElement.current; + if (instanceRef !== undefined) { + instanceRef.current = chartElement.current; } return (): void => { if (chartElement.current === null) return; chartElement.current.dispose(); chartElement.current = null; }; - }, [_instance, onChartInitialized, theme, renderer]); + }, [instanceRef, onChartInitialized, theme, renderer]); // When syncGroup is explicitly set, charts within same group share interactions such as crosshair useEffect(() => { @@ -210,16 +210,21 @@ export const EChart = memo(function EChart({ prevOption.current = option; }, [option]); - // Resize chart, cleanup listener on unmount + // Observe actual container size, including layout and style changes. useLayoutEffect(() => { + const container = containerRef.current; + if (!container) return; const updateSize = debounce(() => { - if (!chartElement.current) return; - chartElement.current.resize(); + chartElement.current?.resize(); }, 200); + const observer = new ResizeObserver(updateSize); + observer.observe(container); window.addEventListener('resize', updateSize); updateSize(); return (): void => { + observer.disconnect(); window.removeEventListener('resize', updateSize); + updateSize.cancel(); }; }, []); @@ -237,26 +242,6 @@ export const EChart = memo(function EChart({ }; }, [onEvents]); - // TODO: re-evaluate how this is triggered. It's technically working right - // now because the sx prop is an object that gets re-created, but that also - // means it runs unnecessarily some of the time and theoretically might - // not run in some other cases. Maybe it should use a resize observer? - useEffect(() => { - // TODO: fix this debouncing. This likely isn't working as intended because - // the debounced function is re-created every time this useEffect is called. - const updateSize = debounce( - () => { - if (!chartElement.current) return; - chartElement.current.resize(); - }, - 200, - { - leading: true, - }, - ); - updateSize(); - }, [sx, style]); - return ; }); diff --git a/components/src/Legend/TableLegend.tsx b/components/src/Legend/TableLegend.tsx index 9abc2a30..8bc7cb2d 100644 --- a/components/src/Legend/TableLegend.tsx +++ b/components/src/Legend/TableLegend.tsx @@ -39,7 +39,7 @@ const COLUMNS: Array> = [ // Starting with `title` attr instead of a tooltip because it is easier to // implement. We should try adding a tooltip in the future, but we'll need // to be very careful about performance when doing so with large tables. - cell: ({ getValue }) => {getValue()}, + cell: ({ getValue }) => ()}>{getValue()}, }, ]; diff --git a/components/src/SettingsAutocomplete/SettingsAutocomplete.tsx b/components/src/SettingsAutocomplete/SettingsAutocomplete.tsx index 985e6a1e..312286dd 100644 --- a/components/src/SettingsAutocomplete/SettingsAutocomplete.tsx +++ b/components/src/SettingsAutocomplete/SettingsAutocomplete.tsx @@ -11,10 +11,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -import type { AutocompleteProps, UseAutocompleteProps } from '@mui/material'; +import type { AutocompleteProps, AutocompleteRenderInputParams, UseAutocompleteProps } from '@mui/material'; import { Autocomplete, TextField, Typography, createFilterOptions } from '@mui/material'; import type { ReactElement, ReactNode } from 'react'; +function renderDefaultInput(params: AutocompleteRenderInputParams): ReactElement { + return ; +} + /** * Interface to extend from for `options` when using `SettingsAutocomplete`. */ @@ -67,7 +71,7 @@ export function SettingsAutocomplete< DisableClearable extends boolean | undefined = false, >({ options, - renderInput = (params): ReactElement => , + renderInput = renderDefaultInput, id, 'aria-labelledby': ariaLabelledby, ...otherProps diff --git a/components/src/Table/Table.test.tsx b/components/src/Table/Table.test.tsx index 32493274..5565293c 100644 --- a/components/src/Table/Table.test.tsx +++ b/components/src/Table/Table.test.tsx @@ -62,7 +62,7 @@ const COLUMNS: TableProps['columns'] = [ header: 'Color', headerDescription: 'Hex codes for colors', width: 100, - cell: ({ getValue }) =>
{getValue()}
, + cell: ({ getValue }) =>
{getValue()}
, enableSorting: true, }, ]; diff --git a/components/src/Table/Table.tsx b/components/src/Table/Table.tsx index cafaa09a..a192a675 100644 --- a/components/src/Table/Table.tsx +++ b/components/src/Table/Table.tsx @@ -80,6 +80,8 @@ export function Table({ defaultColumnConfig, ...otherProps }: TableProps): ReactElement { + 'use no memo'; // TanStack Table v8 uses a mutable instance that automatic memoization cannot track. + const theme = useTheme(); const hasSubRows = !!getSubRows; @@ -205,6 +207,8 @@ export function Table({ return initTableColumns; }, [columns, defaultColumnConfig, hasItemActions, checkboxSelection, actionsColumn, checkboxColumn]); + // Oxlint still checks incompatible APIs inside functions opted out of compilation above. + // oxlint-disable-next-line react/incompatible-library const table = useReactTable({ data, columns: tableColumns, diff --git a/components/src/Table/TableToolbar.tsx b/components/src/Table/TableToolbar.tsx index cebd24bd..f25e8706 100644 --- a/components/src/Table/TableToolbar.tsx +++ b/components/src/Table/TableToolbar.tsx @@ -86,6 +86,8 @@ export function TableToolbar({ isAllExpanded, onExpandAllChange, }: TableToolbarProps): ReactElement | null { + 'use no memo'; // TanStack Table columns retain their identity when visibility changes. + const [colMenuAnchor, setColMenuAnchor] = useState(null); const colMenuOpen = Boolean(colMenuAnchor); const [searchResetKey, setSearchResetKey] = useState(0); diff --git a/components/src/Table/VirtualizedTable.tsx b/components/src/Table/VirtualizedTable.tsx index 85e70ca0..e7f320d1 100644 --- a/components/src/Table/VirtualizedTable.tsx +++ b/components/src/Table/VirtualizedTable.tsx @@ -79,14 +79,14 @@ export function VirtualizedTable({ rows, columns, headers, - columnSizing, - columnSizingInfo, cellConfigs, pagination, onPaginationChange, rowCount, toolbarConfig, }: VirtualizedTableProps): ReactElement { + 'use no memo'; // TanStack Table row and column getters read mutable state behind stable identities. + const virtuosoRef = useRef(null); // Use a ref for these values because they are only needed for keyboard // focus interactions and setting them on state will lead to a significant @@ -133,8 +133,7 @@ export function VirtualizedTable({ }, TableHead, TableFoot, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - TableRow: ({ item, ...props }): ReactElement | null => { + TableRow: ({ item: _item, ...props }): ReactElement | null => { const index = props['data-index']; const row = rows[index]; if (!row) { @@ -183,23 +182,18 @@ export function VirtualizedTable({ /** * Instead of calling `column.getSize()` on every render for every header * and especially every data cell (very expensive), - * we will calculate all column sizes at once at the root table level in a useMemo + * we calculate all column sizes once per render at the root table level * and pass the column sizes down as CSS variables to the element. */ - const columnSizeVars = useMemo(() => { - const colSizes: { [key: string]: number } = {}; - headers.forEach((headerGroup) => { - headerGroup.headers - .filter((header) => header.column.getCanResize()) - .forEach((header) => { - colSizes[`--header-${header.id}-size`] = header.getSize(); - colSizes[`--col-${header.column.id}-size`] = header.column.getSize(); - }); - }); - return colSizes; - // We want to recalculate column sizes whenever column sizes or column resizing info changes. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [columnSizingInfo, columnSizing, headers]); + const columnSizeVars: { [key: string]: number } = {}; + headers.forEach((headerGroup) => { + headerGroup.headers + .filter((header) => header.column.getCanResize()) + .forEach((header) => { + columnSizeVars[`--header-${header.id}-size`] = header.getSize(); + columnSizeVars[`--col-${header.column.id}-size`] = header.column.getSize(); + }); + }); return ( diff --git a/components/src/Table/model/table-model.test.tsx b/components/src/Table/model/table-model.test.tsx index aa5d5292..40cc5c3a 100644 --- a/components/src/Table/model/table-model.test.tsx +++ b/components/src/Table/model/table-model.test.tsx @@ -24,8 +24,7 @@ describe('getTableCellLayout', () => { { name: 'first column', opts: { isFirstColumn: true } }, { name: 'center column', opts: {} }, { name: 'last column', opts: { isLastColumn: true } }, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - ])(`in $name`, ({ name, opts }) => { + ])(`in $name`, ({ opts }) => { expect(getTableCellLayout(mockMuiTheme, density, opts)).toMatchSnapshot(); }); }); @@ -267,7 +266,7 @@ describe('persesColumnToTanstackColumn', () => { headerDescription: 'The total number of values.', width: 120, dataLink: undefined, - cell: (data) => {data.getValue()}, + cell: (data) => {data.getValue()}, cellDescription: (data) => `Desc for ${data.getValue()}`, }, ]; diff --git a/components/src/Table/model/table-model.ts b/components/src/Table/model/table-model.ts index 37f25618..640ffeab 100644 --- a/components/src/Table/model/table-model.ts +++ b/components/src/Table/model/table-model.ts @@ -351,7 +351,6 @@ export interface TableCellConfig { // I couldn't get it to work properly that way and punted on figuring it out // after trying several things. declare module '@tanstack/table-core' { - // eslint-disable-next-line @typescript-eslint/no-unused-vars interface ColumnMeta { align?: TableColumnConfig['align']; headerDescription?: TableColumnConfig['headerDescription']; @@ -375,13 +374,14 @@ export type DataLink = Omit; // Only exposing a very simplified version of the very extensive column definitions // possible with tanstack table to make it easier for us to control rendering // and functionality. -export interface TableColumnConfig - // Any needed to work around some typing issues with tanstack query. - // https://github.com/TanStack/table/issues/4241 - // TODO: revisit issue thread and see if there are any workarounds we can - // use. - // eslint-disable-next-line @typescript-eslint/no-explicit-any - extends Pick, 'accessorKey' | 'cell' | 'sortingFn' | 'id' | 'enableResizing'> { +/** + * Cell values default to `unknown`. In cell callbacks, use `getValue()` + * when the column's value type is known, or narrow the value before using it. + */ +export interface TableColumnConfig extends Pick< + AccessorKeyColumnDef, + 'accessorKey' | 'cell' | 'sortingFn' | 'id' | 'enableResizing' +> { /** * Text to display in the header for the column. */ @@ -400,9 +400,7 @@ export interface TableColumnConfig * If set to `true`, it will use the value generated by the `cell` prop if it * can be treated as a string. */ - // `any` needed for same reason as no-explicit-any higher up in this type. - // eslint-disable-next-line @typescript-eslint/no-explicit-any - cellDescription?: ((props: CellContext) => string) | boolean | undefined; + cellDescription?: ((props: CellContext) => string) | boolean | undefined; /** * When `true`, the column will be sortable. diff --git a/components/src/ThresholdsEditor/ThresholdsEditor.tsx b/components/src/ThresholdsEditor/ThresholdsEditor.tsx index e10d7615..21b7949e 100644 --- a/components/src/ThresholdsEditor/ThresholdsEditor.tsx +++ b/components/src/ThresholdsEditor/ThresholdsEditor.tsx @@ -54,7 +54,7 @@ export function ThresholdsEditor({ if (!recentlyAddedInputRef.current || !focusRef.current) return; recentlyAddedInputRef.current?.focus(); focusRef.current = false; - }, [steps?.length]); + }); const handleThresholdValueChange = (e: React.ChangeEvent, i: number): void => { if (thresholds !== undefined) { diff --git a/components/src/TimeRangeSelector/DateTimeRangePicker.tsx b/components/src/TimeRangeSelector/DateTimeRangePicker.tsx index abab6a85..b7875af6 100644 --- a/components/src/TimeRangeSelector/DateTimeRangePicker.tsx +++ b/components/src/TimeRangeSelector/DateTimeRangePicker.tsx @@ -46,7 +46,7 @@ export const DateTimeRangePicker = ({ timeZone, }: AbsoluteTimeFormProps): ReactElement => { const stdTimeZone = ['local', 'browser'].includes(timeZone.toLowerCase()) - ? Intl.DateTimeFormat().resolvedOptions().timeZone + ? new Intl.DateTimeFormat().resolvedOptions().timeZone : timeZone; const [timeRange, setTimeRange] = useState(initialTimeRange); const [showStartCalendar, setShowStartCalendar] = useState(true); diff --git a/components/src/TimeRangeSelector/TimeRangeSelector.tsx b/components/src/TimeRangeSelector/TimeRangeSelector.tsx index 174be5ab..ff80e913 100644 --- a/components/src/TimeRangeSelector/TimeRangeSelector.tsx +++ b/components/src/TimeRangeSelector/TimeRangeSelector.tsx @@ -17,7 +17,7 @@ import { isRelativeTimeRange, toAbsoluteTimeRange } from '@perses-dev/spec'; import Calendar from 'mdi-material-ui/Calendar'; import EarthIcon from 'mdi-material-ui/Earth'; import type { ReactElement } from 'react'; -import { useMemo, useRef, useState } from 'react'; +import { useMemo, useState } from 'react'; import { useTimeZone } from '../context'; import type { TimeOption } from '../model'; @@ -76,7 +76,7 @@ export function TimeRangeSelector({ const { timeZone: ctxTimeZone } = useTimeZone(); const timeZone = timeZoneProp ?? ctxTimeZone; - const anchorEl = useRef(); + const [anchorEl, setAnchorEl] = useState(null); const [showCustomDateSelector, setShowCustomDateSelector] = useState(false); const convertedTimeRange = useMemo(() => { @@ -130,7 +130,7 @@ export function TimeRangeSelector({ setShowCustomDateSelector(false)} @@ -147,7 +147,7 @@ export function TimeRangeSelector({ timeZone={timeZone} /> - +