From 3154c801ab10476c0499e1a7d0873a99bbee7f36 Mon Sep 17 00:00:00 2001 From: Welkin Wong Date: Sat, 12 Apr 2025 23:39:10 +0800 Subject: [PATCH] fix: #443 useSyncEffect Breaks useFind Reactivity Under Suspense In the client-side implementation of suspense/useFind, useFindClient is used. PR #433 introduced useSyncEffect. Nevertheless, when used in Strict Mode and with Suspense, the useSyncEffect implementation results in non-trivial and hard-to-predict issues. As a result, the Suspense-enabled useFindSuspenseClient will no longer reference useFindClient and will instead be a standalone implementation (derived from the last stable version of useFindClient). --- .../suspense/useFind.tests.js | 154 ++++++++++----- .../react-meteor-data/suspense/useFind.ts | 178 ++++++++++++++++-- 2 files changed, 267 insertions(+), 65 deletions(-) diff --git a/packages/react-meteor-data/suspense/useFind.tests.js b/packages/react-meteor-data/suspense/useFind.tests.js index b82788ce..92995ff6 100644 --- a/packages/react-meteor-data/suspense/useFind.tests.js +++ b/packages/react-meteor-data/suspense/useFind.tests.js @@ -1,85 +1,141 @@ /* global Meteor, Tinytest */ -import React, { Suspense } from 'react' -import { renderToString } from 'react-dom/server' -import { Mongo } from 'meteor/mongo' +import React, { Suspense } from 'react'; +import { renderToString } from 'react-dom/server'; +import { Mongo } from 'meteor/mongo'; +import { renderHook } from '@testing-library/react'; +import { useFindSuspenseClient, useFindSuspenseServer } from './useFind'; + +/** + * Test for useFindSuspenseClient + */ +if (Meteor.isClient) { + Tinytest.addAsync( + 'suspense/useFindSuspenseClient - Verify reference stability between rerenders', + async (test) => { + const TestDocs = new Mongo.Collection(null); + + TestDocs.insert({ id: 0, updated: 0 }); + TestDocs.insert({ id: 1, updated: 0 }); + + const { result, rerender } = renderHook(() => + useFindSuspenseClient(TestDocs, [{}]) + ); + + test.equal( + result.current.length, + 2, + '2 items should have rendered, only 2, no more.' + ); + + await TestDocs.updateAsync({ id: 1 }, { $inc: { updated: 1 } }); + + rerender(); + + test.equal( + result.current.length, + 2, + '2 items should have rendered - only 1 of the items should have been matched by the reconciler after a single change.' + ); + } + ); -import { useFindSuspense } from './useFind' + Tinytest.addAsync( + 'suspense/useFindSuspenseClient - null return is allowed', + async (test) => { + const TestDocs = new Mongo.Collection(null); + + TestDocs.insertAsync({ id: 0, updated: 0 }); + + const { result } = renderHook(() => + useFindSuspenseClient(TestDocs, null) + ); + test.isNull( + result.current, + 'Return value should be null when the factory returns null' + ); + } + ); +} + +/** + * Test for useFindSuspenseServer + */ if (Meteor.isServer) { Tinytest.addAsync( - 'suspense/useFind - Data query validation', + 'suspense/useFindSuspenseServer - Data query validation', async function (test) { - const TestDocs = new Mongo.Collection(null) + const TestDocs = new Mongo.Collection(null); - TestDocs.insertAsync({ id: 0, updated: 0 }) + TestDocs.insertAsync({ id: 0, updated: 0 }); - let returnValue + let returnValue; const Test = () => { - returnValue = useFindSuspense(TestDocs, [{}]) + returnValue = useFindSuspenseServer(TestDocs, [{}]); - return null - } + return null; + }; const TestSuspense = () => { return ( Loading...}> - ) - } + ); + }; // first return promise - renderToString() + renderToString(); test.isUndefined( returnValue, 'Return value should be undefined as find promise unresolved' ); // wait promise - await new Promise((resolve) => setTimeout(resolve, 100)) + await new Promise((resolve) => setTimeout(resolve, 100)); // return data - renderToString() + renderToString(); test.equal( returnValue.length, 1, 'Return value should be an array with one document' - ) + ); } - ) + ); Tinytest.addAsync( - 'suspense/useFind - Test proper cache invalidation', + 'suspense/useFindSuspenseServer - Test proper cache invalidation', async function (test) { - const TestDocs = new Mongo.Collection(null) + const TestDocs = new Mongo.Collection(null); - TestDocs.insertAsync({ id: 0, updated: 0 }) + TestDocs.insertAsync({ id: 0, updated: 0 }); - let returnValue + let returnValue; const Test = () => { - returnValue = useFindSuspense(TestDocs, [{}]) + returnValue = useFindSuspenseServer(TestDocs, [{}]); - return null - } + return null; + }; const TestSuspense = () => { return ( Loading...}> - ) - } + ); + }; // first return promise - renderToString() + renderToString(); test.isUndefined( returnValue, 'Return value should be undefined as find promise unresolved' ); // wait promise - await new Promise((resolve) => setTimeout(resolve, 100)) + await new Promise((resolve) => setTimeout(resolve, 100)); // return data - renderToString() + renderToString(); test.equal( returnValue[0].updated, @@ -87,11 +143,11 @@ if (Meteor.isServer) { 'Return value should be an array with initial value as find promise resolved' ); - TestDocs.updateAsync({ id: 0 }, { $inc: { updated: 1 } }) - await new Promise((resolve) => setTimeout(resolve, 100)) + TestDocs.updateAsync({ id: 0 }, { $inc: { updated: 1 } }); + await new Promise((resolve) => setTimeout(resolve, 100)); // second return promise - renderToString() + renderToString(); test.equal( returnValue[0].updated, @@ -100,46 +156,46 @@ if (Meteor.isServer) { ); // wait promise - await new Promise((resolve) => setTimeout(resolve, 100)) + await new Promise((resolve) => setTimeout(resolve, 100)); // return data - renderToString() + renderToString(); test.equal( returnValue[0].updated, 1, 'Return value should be an array with one document with value updated' - ) + ); } - ) + ); Tinytest.addAsync( - 'suspense/useFind - null return is allowed', + 'suspense/useFindSuspenseServer - null return is allowed', async function (test) { - const TestDocs = new Mongo.Collection(null) + const TestDocs = new Mongo.Collection(null); - TestDocs.insertAsync({ id: 0, updated: 0 }) + TestDocs.insertAsync({ id: 0, updated: 0 }); - let returnValue + let returnValue; const Test = () => { - returnValue = useFindSuspense(TestDocs, null) + returnValue = useFindSuspenseServer(TestDocs, null); - return null - } + return null; + }; const TestSuspense = () => { return ( Loading...}> - ) - } + ); + }; - renderToString() + renderToString(); test.isNull( returnValue, 'Return value should be null when the factory returns null' - ) + ); } - ) + ); } diff --git a/packages/react-meteor-data/suspense/useFind.ts b/packages/react-meteor-data/suspense/useFind.ts index d87a2923..4266a032 100644 --- a/packages/react-meteor-data/suspense/useFind.ts +++ b/packages/react-meteor-data/suspense/useFind.ts @@ -1,10 +1,162 @@ import { Meteor } from 'meteor/meteor' import { EJSON } from 'meteor/ejson' -import { type Mongo } from 'meteor/mongo' +import { Mongo } from 'meteor/mongo' import type React from 'react' -import { useFind as useFindClient } from '../useFind' +import { useReducer, useMemo, useEffect, Reducer, DependencyList, useRef } from 'react' +import { Tracker } from 'meteor/tracker' -const cacheMap = new Map, Map>() +type useFindActions = + | { type: 'refresh' data: T[] } + | { type: 'addedAt' document: T atIndex: number } + | { type: 'changedAt' document: T atIndex: number } + | { type: 'removedAt' atIndex: number } + | { type: 'movedTo' fromIndex: number toIndex: number } + +const useFindReducer = (data: T[], action: useFindActions): T[] => { + switch (action.type) { + case 'refresh': + return action.data + case 'addedAt': + return [ + ...data.slice(0, action.atIndex), + action.document, + ...data.slice(action.atIndex), + ] + case 'changedAt': + return [ + ...data.slice(0, action.atIndex), + action.document, + ...data.slice(action.atIndex + 1), + ] + case 'removedAt': + return [ + ...data.slice(0, action.atIndex), + ...data.slice(action.atIndex + 1), + ] + case 'movedTo': + const doc = data[action.fromIndex] + const copy = [ + ...data.slice(0, action.fromIndex), + ...data.slice(action.fromIndex + 1), + ] + copy.splice(action.toIndex, 0, doc) + return copy + } +} + +// Check for valid Cursor or null. +// On client, we should have a Mongo.Cursor (defined in +// https://github.com/meteor/meteor/blob/devel/packages/minimongo/cursor.js and +// https://github.com/meteor/meteor/blob/devel/packages/mongo/collection.js). +// On server, however, we instead get a private Cursor type from +// https://github.com/meteor/meteor/blob/devel/packages/mongo/mongo_driver.js +// which has fields _mongo and _cursorDescription. +const checkCursor = ( + cursor: + | Mongo.Cursor + | Partial<{ _mongo: any _cursorDescription: any }> + | undefined + | null +) => { + if ( + cursor !== null && + cursor !== undefined && + !(cursor instanceof Mongo.Cursor) && + !(cursor._mongo && cursor._cursorDescription) + ) { + console.warn( + 'Warning: useFind requires an instance of Mongo.Cursor. ' + + 'Make sure you do NOT call .fetch() on your cursor.' + ) + } +} + +// Synchronous data fetch. It uses cursor observing instead of cursor.fetch() because synchronous fetch will be deprecated. +const fetchData = (cursor: Mongo.Cursor) => { + const data: T[] = [] + const observer = cursor.observe({ + addedAt(document, atIndex, before) { + data.splice(atIndex, 0, document) + }, + }) + observer.stop() + return data +} + +export const useFindSuspenseClient = ( + collection: Mongo.Collection, + findArgs: Parameters['find']> | null, + deps: DependencyList = [] +) => { + const findArgsKey = EJSON.stringify(findArgs) + + const cursor = useMemo(() => { + // To avoid creating side effects in render, opt out + // of Tracker integration altogether. + const cursor = Tracker.nonreactive(() => findArgs && collection.find(...findArgs)) + if (Meteor.isDevelopment) { + checkCursor(cursor) + } + return cursor + }, [findArgsKey, ...deps]) + + const [data, dispatch] = useReducer>, null>( + useFindReducer, + null, + () => { + if (!(cursor instanceof Mongo.Cursor)) { + return [] + } + + return fetchData(cursor) + } + ) + + // Store information about mounting the component. + // It will be used to run code only if the component is updated. + const didMount = useRef(false) + + useEffect(() => { + // Fetch intitial data if cursor was changed. + if (didMount.current) { + if (!(cursor instanceof Mongo.Cursor)) { + return + } + + const data = fetchData(cursor) + dispatch({ type: 'refresh', data }) + } else { + didMount.current = true + } + + if (!(cursor instanceof Mongo.Cursor)) { + return + } + + const observer = cursor.observe({ + addedAt(document, atIndex, before) { + dispatch({ type: 'addedAt', document, atIndex }) + }, + changedAt(newDocument, oldDocument, atIndex) { + dispatch({ type: 'changedAt', document: newDocument, atIndex }) + }, + removedAt(oldDocument, atIndex) { + dispatch({ type: 'removedAt', atIndex }) + }, + movedTo(document, fromIndex, toIndex, before) { + dispatch({ type: 'movedTo', fromIndex, toIndex }) + }, + // @ts-ignore + _suppress_initial: true, + }) + + return () => { + observer.stop() + } + }, [cursor]) + + return cursor ? data : cursor +} interface Entry { findArgs: Parameters['find']> @@ -13,14 +165,14 @@ interface Entry { error?: unknown } -const useFindSuspense = ( +const cacheMap = new Map, Map>() + +export const useFindSuspenseServer = ( collection: Mongo.Collection, findArgs: Parameters['find']> | null, deps: React.DependencyList = [] ) => { if (findArgs === null) return null - if (Meteor.isClient) - throw new Error('useFindSuspense is only available on the server.') const cachedEntries = cacheMap.get(collection) const findArgsKey = EJSON.stringify(findArgs) @@ -59,15 +211,9 @@ const useFindSuspense = ( throw entry.promise } -export { useFindSuspense } - -export const useFind = Meteor.isClient - ? ( - collection: Mongo.Collection, - findArgs: Parameters["find"]> | null, - deps?: React.DependencyList - ) => useFindClient(() => findArgs && collection.find(...findArgs), deps) - : useFindSuspense; +export const useFind = Meteor.isServer + ? useFindSuspenseServer + : useFindSuspenseClient function useFindDev( collection: Mongo.Collection, @@ -85,7 +231,7 @@ function useFindDev( warn('Mongo Collection', '1st', 'reactiveFn', collection) } - return useFindSuspense(collection, findArgs, deps) + return useFind(collection, findArgs, deps) } export default Meteor.isDevelopment