presence-related fixes (#1361)

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.
This commit is contained in:
David Sheldrick 2023-05-12 10:43:51 +01:00 committed by GitHub
parent 9ccd0f480f
commit d76446646c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 128 additions and 15 deletions

View file

@ -5050,7 +5050,10 @@ export class App extends EventEmitter<TLEventMap> {
const newPage = TLPage.create({ const newPage = TLPage.create({
id, id,
name: title, name: title,
index: bottomIndex ? getIndexBetween(topIndex, bottomIndex) : getIndexAbove(topIndex), index:
bottomIndex && topIndex !== bottomIndex
? getIndexBetween(topIndex, bottomIndex)
: getIndexAbove(topIndex),
}) })
const newCamera = TLCamera.create({}) const newCamera = TLCamera.create({})

View file

@ -1,3 +1,4 @@
import { TLPage } from '@tldraw/tlschema'
import { MAX_PAGES } from '../../constants' import { MAX_PAGES } from '../../constants'
import { TestApp } from '../TestApp' 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) 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)
})

View file

@ -1,3 +1,4 @@
import { throttledRaf } from '@tldraw/utils'
import { atom, Atom, computed, Computed, Reactor, reactor, transact } from 'signia' import { atom, Atom, computed, Computed, Reactor, reactor, transact } from 'signia'
import { BaseRecord, ID } from './BaseRecord' import { BaseRecord, ID } from './BaseRecord'
import { Cache } from './Cache' import { Cache } from './Cache'
@ -172,7 +173,7 @@ export class Store<R extends BaseRecord = BaseRecord, Props = unknown> {
// If we have accumulated history, flush it and update listeners // If we have accumulated history, flush it and update listeners
this._flushHistory() this._flushHistory()
}, },
{ scheduleEffect: (cb) => requestAnimationFrame(cb) } { scheduleEffect: (cb) => throttledRaf(cb) }
) )
} }

View file

@ -469,6 +469,9 @@ describe('Store', () => {
}) })
it('flushes history before attaching listeners', async () => { it('flushes history before attaching listeners', async () => {
try {
// @ts-expect-error
globalThis.__FORCE_RAF_IN_TESTS__ = true
store.put([Author.create({ name: 'J.R.R Tolkein', id: Author.createCustomId('tolkein') })]) store.put([Author.create({ name: 'J.R.R Tolkein', id: Author.createCustomId('tolkein') })])
const firstListener = jest.fn() const firstListener = jest.fn()
store.listen(firstListener) store.listen(firstListener)
@ -489,6 +492,10 @@ describe('Store', () => {
expect(firstListener).toHaveBeenCalledTimes(1) expect(firstListener).toHaveBeenCalledTimes(1)
expect(secondListener).toHaveBeenCalledTimes(0) expect(secondListener).toHaveBeenCalledTimes(0)
} finally {
// @ts-expect-error
globalThis.__FORCE_RAF_IN_TESTS__ = false
}
}) })
it('does not overwrite default properties with undefined', () => { it('does not overwrite default properties with undefined', () => {

View file

@ -122,6 +122,9 @@ export function promiseWithResolve<T>(): Promise<T> & {
reject: (reason?: any) => void; reject: (reason?: any) => void;
}; };
// @internal
export function rafThrottle(fn: () => void): () => void;
// @public (undocumented) // @public (undocumented)
export type Result<T, E> = ErrorResult<E> | OkResult<T>; export type Result<T, E> = ErrorResult<E> | OkResult<T>;
@ -144,6 +147,9 @@ export { structuredClone_2 as structuredClone }
// @public // @public
export function throttle<T extends (...args: any) => any>(func: T, limit: number): (...args: Parameters<T>) => ReturnType<T>; export function throttle<T extends (...args: any) => any>(func: T, limit: number): (...args: Parameters<T>) => ReturnType<T>;
// @internal
export function throttledRaf(fn: () => void): void;
// (No @packageDocumentation comment for this package) // (No @packageDocumentation comment for this package)
``` ```

View file

@ -23,4 +23,5 @@ export {
objectMapKeys, objectMapKeys,
objectMapValues, objectMapValues,
} from './lib/object' } from './lib/object'
export { rafThrottle, throttledRaf } from './lib/raf'
export { isDefined, isNonNull, isNonNullish, structuredClone } from './lib/value' export { isDefined, isNonNull, isNonNullish, structuredClone } from './lib/value'

View file

@ -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()
}