From 8b729117c45be7132dace3c44c516c7978cbd2b0 Mon Sep 17 00:00:00 2001 From: Cody Olsen Date: Fri, 14 Jun 2024 19:09:17 +0200 Subject: [PATCH] refactor: remove debug code and previous implementation --- package.json | 2 +- .../react-track-elements/{v2.tsx => hooks.ts} | 44 +--- .../components/react-track-elements/index.ts | 3 +- .../components/react-track-elements/v1.tsx | 205 ------------------ .../src/core/presence/overlay/tracker.tsx | 31 +-- 5 files changed, 14 insertions(+), 271 deletions(-) rename packages/sanity/src/core/components/react-track-elements/{v2.tsx => hooks.ts} (71%) delete mode 100644 packages/sanity/src/core/components/react-track-elements/v1.tsx diff --git a/package.json b/package.json index cf4b75342687..e42925b95f70 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "check:deps": "pnpm --recursive --parallel exec depcheck", "check:format": "prettier . --check", "check:lint": "turbo run lint --continue -- --quiet", - "check:react-compiler": "eslint --no-inline-config --no-eslintrc --ext .cjs,.mjs,.js,.jsx,.ts,.tsx --parser @typescript-eslint/parser --plugin react-compiler --rule 'react-compiler/react-compiler: [warn]' --ignore-path .eslintignore.react-compiler --max-warnings 30 .", + "check:react-compiler": "eslint --no-inline-config --no-eslintrc --ext .cjs,.mjs,.js,.jsx,.ts,.tsx --parser @typescript-eslint/parser --plugin react-compiler --rule 'react-compiler/react-compiler: [warn]' --ignore-path .eslintignore.react-compiler --max-warnings 29 .", "check:test": "run-s test -- --silent", "check:types": "tsc && turbo run check:types --filter='./packages/*' --filter='./packages/@sanity/*'", "chore:format:fix": "prettier --cache --write .", diff --git a/packages/sanity/src/core/components/react-track-elements/v2.tsx b/packages/sanity/src/core/components/react-track-elements/hooks.ts similarity index 71% rename from packages/sanity/src/core/components/react-track-elements/v2.tsx rename to packages/sanity/src/core/components/react-track-elements/hooks.ts index a289c3ab7bf0..000d53caab66 100644 --- a/packages/sanity/src/core/components/react-track-elements/v2.tsx +++ b/packages/sanity/src/core/components/react-track-elements/hooks.ts @@ -1,6 +1,5 @@ -/* eslint-disable no-console */ import {debounce} from 'lodash' -import {useEffect, useLayoutEffect, useMemo, useReducer, useRef, useState} from 'react' +import {useLayoutEffect, useMemo, useReducer, useRef, useState} from 'react' import {type IsEqualFunction} from './types' @@ -12,6 +11,14 @@ export interface TrackerContextStore { } function createStore(reportedValues: Map, publish: () => void) { + /** + * This implementation is over 4 years old, and is part of tackling a hard problem: + * tracking the position of DOM nodes efficiently, so that Presence Sticky Overlays can render correctly and respond to scroll, + * and so that Change Indicator connectors can draw paths that traces a document change to its form input field no matter how they layout shifts. + * 4 years ago we didn't have a lot of options when solving this problem. + * But today we have great success with using `@floating-ui/react` in `@sanity/ui` with a very similar problem: positioning tooltips and popovers correctly no matter how the page scrolls or the layout shifts. + * We should consider migrating to `@floating-ui/react` for this problem as well. + */ function add(id: string, value: Value) { if (reportedValues.has(id)) { // eslint-disable-next-line no-console @@ -64,10 +71,6 @@ export function useTrackerStore(): { [debouncedUpdateSnapshot, reportedValues], ) - useEffect(() => { - console.log('useTrackerStore.useEffect') - }, []) - return {store, snapshot} } @@ -86,20 +89,13 @@ export function useTrackerStoreReporter( * Setup and teardown, only runs if `id`, `store` or the `value` getter changes */ if (id === null || store === null) { - console.log('useTrackerStoreReporter.add', 'id is null') return undefined } - console.groupCollapsed(`useTrackerStoreReporter.add(${id})`) - console.count(id) const nextValue = value() - console.log({current: nextValue}) - console.log('previous.current', previousRef.current) store.add(id, nextValue) idRef.current = id previousRef.current = nextValue - console.groupEnd() return () => { - console.count(`useTrackerStoreReporter.remove(${id})`) store.remove(id) idRef.current = null previousRef.current = null @@ -113,38 +109,16 @@ export function useTrackerStoreReporter( * @TODO This is a bit expensive, and we should migrate to using a library like `@floating-ui/react` instead of rolling our own solution. */ if (id === null || idRef.current === null || store === null || id !== idRef.current) { - console.count( - `useTrackerStoreReporter.update(${idRef.current || 'null'}, ${id || 'null'}): skipped`, - ) return undefined } const nextValue = value() if (isEqual(previousRef.current, nextValue)) { - console.count( - `useTrackerStoreReporter.update(${idRef.current || 'null'}, ${id || 'null'}): skipped, equal state`, - ) return undefined } - console.group(`useTrackerStoreReporter.update(${id})`) store.update(id, nextValue) - console.count(`update(id: ${id}, current: ${nextValue})`) - console.log({'previous.current': previousRef.current, 'current': nextValue}) - console.groupEnd() - previousRef.current = nextValue return undefined }) } - -/** @internal */ -export function useTrackerStoreReportedValues( - snapshot: TrackerContextGetSnapshot | null, -): [string, Value][] { - useEffect(() => { - console.log('useTrackerStoreReportedValues.useEffect', {snapshot}) - }, [snapshot]) - - return snapshot || [] -} diff --git a/packages/sanity/src/core/components/react-track-elements/index.ts b/packages/sanity/src/core/components/react-track-elements/index.ts index fd00e7d12e0a..a1c77e913b7d 100644 --- a/packages/sanity/src/core/components/react-track-elements/index.ts +++ b/packages/sanity/src/core/components/react-track-elements/index.ts @@ -1,3 +1,2 @@ +export * from './hooks' export * from './types' -export * from './v1' -export * from './v2' diff --git a/packages/sanity/src/core/components/react-track-elements/v1.tsx b/packages/sanity/src/core/components/react-track-elements/v1.tsx deleted file mode 100644 index c3ed02bf1fd1..000000000000 --- a/packages/sanity/src/core/components/react-track-elements/v1.tsx +++ /dev/null @@ -1,205 +0,0 @@ -/* eslint-disable no-console */ -import {debounce} from 'lodash' -import createPubsub, {type Subscriber} from 'nano-pubsub' -import { - // eslint-disable-next-line no-restricted-imports - createContext, - type ReactNode, - useContext, - useLayoutEffect, - useRef, - useState, -} from 'react' - -import {type IsEqualFunction, type Reported, type ReporterHook} from './types' - -/** @internal */ -export interface TrackerContext { - add: (id: string, value: Value) => void - update: (id: string, value: Value) => void - remove: (id: string) => void - read: () => Reported[] - subscribe: (subscriber: Subscriber[]>) => () => void -} - -function isFunc(value: T | (() => T)): value is () => T { - return typeof value === 'function' -} - -function safeRead(value: T | (() => T)): T { - return isFunc(value) ? value() : value -} - -// eslint-disable-next-line @typescript-eslint/no-empty-function -const noop = () => undefined - -// Todo: consider memozing individual functions or move the context assertion/guard to a separate step. -let didWarn = false -const useReporterGuard = (id: string): void => { - if (!didWarn) { - // eslint-disable-next-line no-console - console.warn( - new Error( - `No context provided for reporter. Make sure that the component calling "useReporter(${id}, ...)", is wrapped in a element`, - ), - ) - } - didWarn = true -} - -function useReportedValueGuard(): Reported[] { - if (!didWarn) { - // eslint-disable-next-line no-console - console.warn( - new Error( - 'No context provided for reporter. Make sure that the component calling "useReportedValues()", is wrapped inside a element', - ), - ) - } - didWarn = true - return [] -} - -const useSubscribeGuard = () => { - if (!didWarn) { - // eslint-disable-next-line no-console - console.warn( - new Error( - 'No context provided for reporter. Make sure that the component calling "useReportedValues()", is wrapped inside a element', - ), - ) - } - didWarn = true - // eslint-disable-next-line @typescript-eslint/no-empty-function - return () => {} -} - -const DEFAULT_CONTEXT: TrackerContext = { - add: useReporterGuard, - update: useReporterGuard, - remove: useReporterGuard, - subscribe: useSubscribeGuard, - read: useReportedValueGuard, -} - -/** @internal */ -export function createTrackerScope(): { - Tracker: (props: {children: ReactNode}) => JSX.Element - useReportedValues: () => Reported[] - useReporter: ReporterHook -} { - const Context = createContext(DEFAULT_CONTEXT as TrackerContext) - - function useReportedValues() { - const context = useContext(Context) - const [values, setValues] = useState(context.read()) - useLayoutEffect(() => { - setValues(context.read()) - return context.subscribe(setValues) - }, [context]) - return values - } - - function Tracker(props: {children: ReactNode}) { - const [store] = useState(() => createStore()) - return {props.children} - } - - function useReporter( - // No reporting will happen if id=null - id: string | null, - value: () => Value, - isEqual: IsEqualFunction = Object.is, - ) { - const {add, update, remove} = useContext(Context) - const previous = useRef() - - useLayoutEffect(() => { - if (id === null) { - console.log('useReporter.add', 'id is null') - return noop - } - console.groupCollapsed(`useReporter.add(${id})`) - console.count(id) - const current = safeRead(value) - console.log({current}) - console.log('previous.current', previous.current) - add(id, current) - previous.current = current - console.groupEnd() - return () => { - console.count(`useReporter.remove(${id})`) - remove(id) - } - }, [add, id, remove, value]) - - useLayoutEffect(() => { - const current = safeRead(value) - if ( - typeof previous.current !== 'undefined' && - !isEqual(previous.current, current) && - id !== null - ) { - console.group(`useReporter.update(${id})`) - update(id, current) - console.count(`update(id: ${id}, current: ${current})`) - console.log({'previous.current': previous.current, current}) - console.groupEnd() - } else { - console.count(`useReporter.update(${id || 'null'}): skipped`) - } - previous.current = current - }) - } - - return { - Tracker, - useReportedValues, - useReporter, - } -} - -function createStore() { - const reportedValues = new Map() - const {publish, subscribe} = createPubsub[]>() - - const debouncedPublish = debounce(publish, 10, {trailing: true}) - const read = () => Array.from(reportedValues.entries()) - - function add(id: string, value: Value) { - if (reportedValues.has(id)) { - // eslint-disable-next-line no-console - // console.error( - // new Error( - // `Invalid call to useReporter(${id}): A component reporting on "${id}" is already mounted in the subtree. Make sure that all reporters within the same subtree have unique ids.` - // ) - // ) - } - reportedValues.set(id, value) - debouncedPublish(read()) - } - - function update(id: string, value: Value) { - if (!reportedValues.has(id)) { - // throw new Error(`A reporter with id "${id}" is not known.`) - } - reportedValues.set(id, value) - debouncedPublish(read()) - } - - function remove(id: string) { - if (!reportedValues.has(id)) { - // throw new Error(`A reporter with id "${id}" is not known`) - } - reportedValues.delete(id) - debouncedPublish(read()) - } - - return { - add, - remove, - update, - read, - subscribe, - } -} diff --git a/packages/sanity/src/core/presence/overlay/tracker.tsx b/packages/sanity/src/core/presence/overlay/tracker.tsx index c7cfc8d4bfe5..8edc9a254187 100644 --- a/packages/sanity/src/core/presence/overlay/tracker.tsx +++ b/packages/sanity/src/core/presence/overlay/tracker.tsx @@ -2,7 +2,6 @@ import {memo, useContext} from 'react' import {PresenceTrackerContextGetSnapshot, PresenceTrackerContextStore} from 'sanity/_singletons' import { - createTrackerScope, type Reported, type ReporterHook, type TrackerContextGetSnapshot, @@ -13,14 +12,6 @@ import {type FieldPresenceData} from '../types' export type ReportedPresenceData = Reported -const variant: '1' | '2' = '2' - -const { - Tracker: PresenceTrackerV1, - useReporter: useReporterV1, - useReportedValues: useReportedValuesV1, -} = createTrackerScope() - function PresenceTrackerComponent(props: {children: React.ReactNode}) { const {children} = props const {store, snapshot} = useTrackerStore() @@ -37,14 +28,14 @@ function PresenceTrackerComponent(props: {children: React.ReactNode}) { /** * @internal */ -export const PresenceTrackerV2 = memo(PresenceTrackerComponent) +export const PresenceTracker = memo(PresenceTrackerComponent) const EMPTY_ARRAY: Reported[] = [] /** * @internal */ -export function usePresenceReportedValuesV2(): TrackerContextGetSnapshot { +export function usePresenceReportedValues(): TrackerContextGetSnapshot { const snapshot = useContext(PresenceTrackerContextGetSnapshot) if (snapshot === null) { @@ -63,7 +54,7 @@ export function usePresenceReportedValuesV2(): TrackerContextGetSnapshot = (id, value, isEqual?) => { +export const usePresenceReporter: ReporterHook = (id, value, isEqual?) => { const store = useContext(PresenceTrackerContextStore) if (store === null) { @@ -77,19 +68,3 @@ export const usePresenceReporterV2: ReporterHook = (id, value useTrackerStoreReporter(store, id, value, isEqual) } - -/** - * @internal - */ -export const PresenceTracker = variant === '1' ? PresenceTrackerV1 : PresenceTrackerV2 - -/** - * @internal - */ -export const usePresenceReportedValues = - variant === '1' ? useReportedValuesV1 : usePresenceReportedValuesV2 - -/** - * @internal - */ -export const usePresenceReporter = variant === '1' ? useReporterV1 : usePresenceReporterV2