From d76446646c51dbb6b998e1ed130444f869335409 Mon Sep 17 00:00:00 2001 From: David Sheldrick Date: Fri, 12 May 2023 10:43:51 +0100 Subject: [PATCH] presence-related fixes (#1361) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a bug where creating a page would fail if there were multiple pages with the same index. This also changes the store to use a throttled version of requestAnimationFrame. This should be good for relieving backpressure in situations where the store is updated many times in quick succession. It also makes testing a lot easier since it has the mocking logic built in. ### Change Type - [x] `patch` — Bug Fix ### Release Notes - Fix a bug where creating a page could throw an error in some multiplayer contexts. --- packages/editor/src/lib/app/App.ts | 5 +- .../src/lib/test/commands/createPage.test.ts | 28 ++++++++ packages/tlstore/src/lib/Store.ts | 3 +- .../tlstore/src/lib/test/recordStore.test.ts | 33 +++++---- packages/utils/api-report.md | 6 ++ packages/utils/src/index.ts | 1 + packages/utils/src/lib/raf.ts | 67 +++++++++++++++++++ 7 files changed, 128 insertions(+), 15 deletions(-) create mode 100644 packages/utils/src/lib/raf.ts diff --git a/packages/editor/src/lib/app/App.ts b/packages/editor/src/lib/app/App.ts index fdb8e923c..396466967 100644 --- a/packages/editor/src/lib/app/App.ts +++ b/packages/editor/src/lib/app/App.ts @@ -5050,7 +5050,10 @@ export class App extends EventEmitter { const newPage = TLPage.create({ id, name: title, - index: bottomIndex ? getIndexBetween(topIndex, bottomIndex) : getIndexAbove(topIndex), + index: + bottomIndex && topIndex !== bottomIndex + ? getIndexBetween(topIndex, bottomIndex) + : getIndexAbove(topIndex), }) const newCamera = TLCamera.create({}) diff --git a/packages/editor/src/lib/test/commands/createPage.test.ts b/packages/editor/src/lib/test/commands/createPage.test.ts index 3816f5924..4c4b65996 100644 --- a/packages/editor/src/lib/test/commands/createPage.test.ts +++ b/packages/editor/src/lib/test/commands/createPage.test.ts @@ -1,3 +1,4 @@ +import { TLPage } from '@tldraw/tlschema' import { MAX_PAGES } from '../../constants' import { TestApp } from '../TestApp' @@ -28,3 +29,30 @@ it("Doesn't create a page if max pages is reached", () => { } expect(app.pages.length).toBe(MAX_PAGES) }) + +it('[regression] does not die if every page has the same index', () => { + expect(app.pages.length).toBe(1) + const page = app.pages[0] + app.store.put([ + { + ...page, + id: TLPage.createCustomId('2'), + name: 'a', + }, + { + ...page, + id: TLPage.createCustomId('3'), + name: 'b', + }, + { + ...page, + id: TLPage.createCustomId('4'), + name: 'c', + }, + ]) + + expect(app.pages.every((p) => p.index === page.index)).toBe(true) + + app.createPage('My Special Test Page') + expect(app.pages.some((p) => p.name === 'My Special Test Page')).toBe(true) +}) diff --git a/packages/tlstore/src/lib/Store.ts b/packages/tlstore/src/lib/Store.ts index ebdd39cb0..6a93df563 100644 --- a/packages/tlstore/src/lib/Store.ts +++ b/packages/tlstore/src/lib/Store.ts @@ -1,3 +1,4 @@ +import { throttledRaf } from '@tldraw/utils' import { atom, Atom, computed, Computed, Reactor, reactor, transact } from 'signia' import { BaseRecord, ID } from './BaseRecord' import { Cache } from './Cache' @@ -172,7 +173,7 @@ export class Store { // If we have accumulated history, flush it and update listeners this._flushHistory() }, - { scheduleEffect: (cb) => requestAnimationFrame(cb) } + { scheduleEffect: (cb) => throttledRaf(cb) } ) } diff --git a/packages/tlstore/src/lib/test/recordStore.test.ts b/packages/tlstore/src/lib/test/recordStore.test.ts index e043f907e..b07333c84 100644 --- a/packages/tlstore/src/lib/test/recordStore.test.ts +++ b/packages/tlstore/src/lib/test/recordStore.test.ts @@ -469,26 +469,33 @@ describe('Store', () => { }) it('flushes history before attaching listeners', async () => { - store.put([Author.create({ name: 'J.R.R Tolkein', id: Author.createCustomId('tolkein') })]) - const firstListener = jest.fn() - store.listen(firstListener) - expect(firstListener).toHaveBeenCalledTimes(0) + try { + // @ts-expect-error + globalThis.__FORCE_RAF_IN_TESTS__ = true + store.put([Author.create({ name: 'J.R.R Tolkein', id: Author.createCustomId('tolkein') })]) + const firstListener = jest.fn() + store.listen(firstListener) + expect(firstListener).toHaveBeenCalledTimes(0) - store.put([Author.create({ name: 'Chips McCoy', id: Author.createCustomId('chips') })]) + store.put([Author.create({ name: 'Chips McCoy', id: Author.createCustomId('chips') })]) - expect(firstListener).toHaveBeenCalledTimes(0) + expect(firstListener).toHaveBeenCalledTimes(0) - const secondListener = jest.fn() + const secondListener = jest.fn() - store.listen(secondListener) + store.listen(secondListener) - expect(firstListener).toHaveBeenCalledTimes(1) - expect(secondListener).toHaveBeenCalledTimes(0) + expect(firstListener).toHaveBeenCalledTimes(1) + expect(secondListener).toHaveBeenCalledTimes(0) - await new Promise((resolve) => requestAnimationFrame(resolve)) + await new Promise((resolve) => requestAnimationFrame(resolve)) - expect(firstListener).toHaveBeenCalledTimes(1) - expect(secondListener).toHaveBeenCalledTimes(0) + expect(firstListener).toHaveBeenCalledTimes(1) + expect(secondListener).toHaveBeenCalledTimes(0) + } finally { + // @ts-expect-error + globalThis.__FORCE_RAF_IN_TESTS__ = false + } }) it('does not overwrite default properties with undefined', () => { diff --git a/packages/utils/api-report.md b/packages/utils/api-report.md index a93ce397b..8f6c1fc8c 100644 --- a/packages/utils/api-report.md +++ b/packages/utils/api-report.md @@ -122,6 +122,9 @@ export function promiseWithResolve(): Promise & { reject: (reason?: any) => void; }; +// @internal +export function rafThrottle(fn: () => void): () => void; + // @public (undocumented) export type Result = ErrorResult | OkResult; @@ -144,6 +147,9 @@ export { structuredClone_2 as structuredClone } // @public export function throttle any>(func: T, limit: number): (...args: Parameters) => ReturnType; +// @internal +export function throttledRaf(fn: () => void): void; + // (No @packageDocumentation comment for this package) ``` diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index 503b9c918..35eb270da 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -23,4 +23,5 @@ export { objectMapKeys, objectMapValues, } from './lib/object' +export { rafThrottle, throttledRaf } from './lib/raf' export { isDefined, isNonNull, isNonNullish, structuredClone } from './lib/value' diff --git a/packages/utils/src/lib/raf.ts b/packages/utils/src/lib/raf.ts new file mode 100644 index 000000000..8078b6191 --- /dev/null +++ b/packages/utils/src/lib/raf.ts @@ -0,0 +1,67 @@ +const isTest = () => + typeof process !== 'undefined' && + process.env.NODE_ENV === 'test' && + // @ts-expect-error + !globalThis.__FORCE_RAF_IN_TESTS__ + +const rafQueue: Array<() => void> = [] + +const tick = () => { + const queue = rafQueue.splice(0, rafQueue.length) + for (const fn of queue) { + fn() + } +} + +let frame: number | undefined + +function raf() { + if (frame) { + return + } + + frame = requestAnimationFrame(() => { + frame = undefined + tick() + }) +} + +/** + * Returns a throttled version of the function that will only be called max once per frame. + * @param fn - the fun to return a throttled version of + * @returns + * @internal + */ +export function rafThrottle(fn: () => void) { + if (isTest()) { + return fn + } + + return () => { + if (rafQueue.includes(fn)) { + return + } + rafQueue.push(fn) + raf() + } +} + +/** + * Calls the function on the next frame. + * If the same fn is passed again before the next frame, it will still be called only once. + * @param fn - the fun to call on the next animation frame + * @returns + * @internal + */ +export function throttledRaf(fn: () => void) { + if (isTest()) { + return fn() + } + + if (rafQueue.includes(fn)) { + return + } + + rafQueue.push(fn) + raf() +}