From fedbfc99add67f2e4c1ef177326bacb09b9ea1b3 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Fri, 2 Jun 2023 15:19:45 +0200 Subject: [PATCH 01/23] Very naive batching implementation Co-authored-by: Omar --- .../src/mapFacets/mapIntoObserveArray.spec.ts | 84 ++++++++++++++++++- .../core/src/mapFacets/mapIntoObserveArray.ts | 34 ++++++-- 2 files changed, 111 insertions(+), 7 deletions(-) diff --git a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.spec.ts b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.spec.ts index 6c10a079..e06f3e44 100644 --- a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.spec.ts +++ b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.spec.ts @@ -1,6 +1,8 @@ import { defaultEqualityCheck } from '../equalityChecks' import { NO_VALUE } from '../types' -import { mapIntoObserveArray } from './mapIntoObserveArray' +import { mapIntoObserveArray, batch } from './mapIntoObserveArray' +import { createFacet } from '../facet' +import { mapFacetSingleLightweight } from './mapFacetSingleLightweight' it('checks equality of primitives when passing defaultEqualityCheck', () => { const sourceA = { observe: jest.fn(), get: jest.fn() } @@ -226,3 +228,83 @@ describe('mapping to NO_VALUE', () => { expect(listener).not.toBeCalled() }) }) + +describe('batch', () => { + it('supports batching', () => { + const facetA = createFacet({ initialValue: 'a1' }) + const facetB = createFacet({ initialValue: 'b1' }) + const observe = mapIntoObserveArray([facetA, facetB], (a, b) => `${a} ${b}`) + + const observer = jest.fn() + observe(observer) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('a1 b1') + + jest.clearAllMocks() + batch(() => { + facetA.set('a2') + facetB.set('b2') + }) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('a2 b2') + }) + + it('supports batching, but nested', () => { + const facetA = createFacet({ initialValue: 'a1' }) + const facetB = createFacet({ initialValue: 'b1' }) + const observe = mapIntoObserveArray([facetA, facetB], (a, b) => `${a} ${b}`) + + const observer = jest.fn() + observe(observer) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('a1 b1') + + jest.clearAllMocks() + batch(() => { + facetA.set('a2') + batch(() => { + facetB.set('b2') + }) + }) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('a2 b2') + }) + + it('batches a single facet mapped into multiple facets and then combined again', () => { + const facet = createFacet({ initialValue: 'a' }) + const first = mapFacetSingleLightweight(facet, (a) => `first ${a}`) + const second = mapFacetSingleLightweight(facet, (a) => `second ${a}`) + + const observe = mapIntoObserveArray([first, second], (a, b) => `${a},${b}`) + + const observer = jest.fn() + observe(observer) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('first a,second a') + + jest.clearAllMocks() + batch(() => { + facet.set('b') + }) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('first b,second b') + }) + + // it('supports composition', () => { + // const facetA = createFacet({ initialValue: 'A' }) + // const facetA_A = mapFacetSingleLightweight(facetA, (a) => `${a}_A`) + // const facetA_B = mapFacetSingleLightweight(facetA, (a) => `${a}_B`) + // }) +}) + +// engine.on('start', () => { +// batch(() => { +// engine.on('cnw', (value) => facet.set(value)) +// }) +// }) diff --git a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts index 1bbe3cc5..9b3a5b16 100644 --- a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts +++ b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts @@ -14,20 +14,27 @@ export function mapIntoObserveArray( const dependencyValues: Option[] = facets.map(() => NO_VALUE) let hasAllDependencies = false + const notify = () => { + const result = fn(...dependencyValues) + if (result === NO_VALUE) return + + listener(result) + } + const subscriptions = facets.map((facet, index) => { // Most common scenario is not having any equality check if (equalityCheck == null) { return facet.observe((value) => { dependencyValues[index] = value - hasAllDependencies = hasAllDependencies || dependencyValues.every((value) => value != NO_VALUE) + if (batchId >= 0) { + scheduledBatches[batchId] = notify + return + } - if (hasAllDependencies) { - const result = fn(...dependencyValues) - if (result === NO_VALUE) return + hasAllDependencies = hasAllDependencies || dependencyValues.every((value) => value != NO_VALUE) - listener(result) - } + if (hasAllDependencies) notify() }) } @@ -86,3 +93,18 @@ export function mapIntoObserveArray( } } } + +export type Cb = () => void + +let batchId = -1 +const scheduledBatches: Cb[] = [] + +export const batch = (cb: Cb) => { + batchId += 1 + + cb() + + scheduledBatches[batchId]() + + batchId -= 1 +} From e86aaf06d15097197f11b98486a4d6ab8f1e0b26 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Fri, 2 Jun 2023 16:07:50 +0200 Subject: [PATCH 02/23] Batches effects of other batches Co-authored-by: Omar --- .../src/mapFacets/mapIntoObserveArray.spec.ts | 49 ++++++++++++++----- .../core/src/mapFacets/mapIntoObserveArray.ts | 19 +++++-- 2 files changed, 52 insertions(+), 16 deletions(-) diff --git a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.spec.ts b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.spec.ts index e06f3e44..76545658 100644 --- a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.spec.ts +++ b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.spec.ts @@ -3,6 +3,7 @@ import { NO_VALUE } from '../types' import { mapIntoObserveArray, batch } from './mapIntoObserveArray' import { createFacet } from '../facet' import { mapFacetSingleLightweight } from './mapFacetSingleLightweight' +import { mapFacetArrayLightweight } from './mapFacetArrayLightweight' it('checks equality of primitives when passing defaultEqualityCheck', () => { const sourceA = { observe: jest.fn(), get: jest.fn() } @@ -278,7 +279,6 @@ describe('batch', () => { const facet = createFacet({ initialValue: 'a' }) const first = mapFacetSingleLightweight(facet, (a) => `first ${a}`) const second = mapFacetSingleLightweight(facet, (a) => `second ${a}`) - const observe = mapIntoObserveArray([first, second], (a, b) => `${a},${b}`) const observer = jest.fn() @@ -296,15 +296,40 @@ describe('batch', () => { expect(observer).toHaveBeenCalledWith('first b,second b') }) - // it('supports composition', () => { - // const facetA = createFacet({ initialValue: 'A' }) - // const facetA_A = mapFacetSingleLightweight(facetA, (a) => `${a}_A`) - // const facetA_B = mapFacetSingleLightweight(facetA, (a) => `${a}_B`) - // }) -}) + it('batches effects of other batches', () => { + const derivativeFacet = createFacet({ initialValue: 'a' }) + const derivativeFacetFirst = mapFacetSingleLightweight(derivativeFacet, (a) => `first ${a}`) + const derivativeFacetSecond = mapFacetSingleLightweight(derivativeFacet, (a) => `second ${a}`) + const derivativeFacetMapped = mapFacetArrayLightweight( + [derivativeFacetFirst, derivativeFacetSecond], + (a, b) => `${a},${b}`, + ) + + const sourceFacetA = createFacet({ initialValue: 'a1' }) + const sourceFacetB = createFacet({ initialValue: 'b1' }) + const mappedFacetAAndFacetB = mapFacetArrayLightweight([sourceFacetA, sourceFacetB], (a, b) => `${a}_${b}`) + + const observer = jest.fn() + mappedFacetAAndFacetB.observe((value) => { + batch(() => { + derivativeFacet.set(value) + }) + }) + + derivativeFacetMapped.observe(observer) -// engine.on('start', () => { -// batch(() => { -// engine.on('cnw', (value) => facet.set(value)) -// }) -// }) + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('first a1_b1,second a1_b1') + + jest.clearAllMocks() + // this batch groups with the batch that's called as an effect of it's function call contents + batch(() => { + sourceFacetA.set('a2') + sourceFacetB.set('b2') + console.log('got here') + }) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('first a2_b2,second a2_b2') + }) +}) diff --git a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts index 9b3a5b16..272f59a1 100644 --- a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts +++ b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts @@ -28,7 +28,7 @@ export function mapIntoObserveArray( dependencyValues[index] = value if (batchId >= 0) { - scheduledBatches[batchId] = notify + scheduledBatches.add(notify) return } @@ -97,14 +97,25 @@ export function mapIntoObserveArray( export type Cb = () => void let batchId = -1 -const scheduledBatches: Cb[] = [] +let scheduledBatches = new Set() export const batch = (cb: Cb) => { batchId += 1 cb() - scheduledBatches[batchId]() - batchId -= 1 + + // We are back at the root batch call + if (batchId === -1) { + // Make a copy of the schedule + // As notifying can start other batch roots + const array = Array.from(scheduledBatches) + scheduledBatches = new Set() + + for (let index = array.length - 1; index >= 0; index--) { + const notify = array[index] + notify() + } + } } From b40aac34fb0fedff3580adc7ef2ed1402cc42fc0 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Fri, 2 Jun 2023 16:39:37 +0200 Subject: [PATCH 03/23] Extract batch to a separated module and make it apply to Facet by default --- packages/@react-facet/core/src/batch.spec.ts | 100 ++++++++++++++++ packages/@react-facet/core/src/batch.ts | 31 +++++ .../core/src/facet/createFacet.ts | 47 ++++---- .../src/mapFacets/mapIntoObserveArray.spec.ts | 109 +----------------- .../core/src/mapFacets/mapIntoObserveArray.ts | 93 ++++----------- 5 files changed, 181 insertions(+), 199 deletions(-) create mode 100644 packages/@react-facet/core/src/batch.spec.ts create mode 100644 packages/@react-facet/core/src/batch.ts diff --git a/packages/@react-facet/core/src/batch.spec.ts b/packages/@react-facet/core/src/batch.spec.ts new file mode 100644 index 00000000..2246ee74 --- /dev/null +++ b/packages/@react-facet/core/src/batch.spec.ts @@ -0,0 +1,100 @@ +import { createFacet } from './facet' +import { mapFacetsLightweight } from './mapFacets' +import { batch } from './batch' + +it('supports batching', () => { + const facetA = createFacet({ initialValue: 'a1' }) + const facetB = createFacet({ initialValue: 'b1' }) + const facetAB = mapFacetsLightweight([facetA, facetB], (a, b) => `${a} ${b}`) + + const observer = jest.fn() + facetAB.observe(observer) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('a1 b1') + + jest.clearAllMocks() + batch(() => { + facetA.set('a2') + facetB.set('b2') + }) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('a2 b2') +}) + +it('supports batching, but nested', () => { + const facetA = createFacet({ initialValue: 'a1' }) + const facetB = createFacet({ initialValue: 'b1' }) + const facetAB = mapFacetsLightweight([facetA, facetB], (a, b) => `${a} ${b}`) + + const observer = jest.fn() + facetAB.observe(observer) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('a1 b1') + + jest.clearAllMocks() + batch(() => { + facetA.set('a2') + batch(() => { + facetB.set('b2') + }) + }) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('a2 b2') +}) + +it('batches a single facet mapped into multiple facets and then combined again', () => { + const facet = createFacet({ initialValue: 'a' }) + const first = mapFacetsLightweight([facet], (a) => `first ${a}`) + const second = mapFacetsLightweight([facet], (a) => `second ${a}`) + const combinedAgain = mapFacetsLightweight([first, second], (a, b) => `${a},${b}`) + + const observer = jest.fn() + combinedAgain.observe(observer) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('first a,second a') + + jest.clearAllMocks() + facet.set('b') + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('first b,second b') +}) + +it('batches effects of other batches', () => { + const derivativeFacet = createFacet({ initialValue: 'a' }) + const derivativeFacetFirst = mapFacetsLightweight([derivativeFacet], (a) => `first ${a}`) + const derivativeFacetSecond = mapFacetsLightweight([derivativeFacet], (a) => `second ${a}`) + const derivativeFacetMapped = mapFacetsLightweight( + [derivativeFacetFirst, derivativeFacetSecond], + (a, b) => `${a},${b}`, + ) + + const sourceFacetA = createFacet({ initialValue: 'a1' }) + const sourceFacetB = createFacet({ initialValue: 'b1' }) + const mappedFacetAAndFacetB = mapFacetsLightweight([sourceFacetA, sourceFacetB], (a, b) => `${a}_${b}`) + + const observer = jest.fn() + mappedFacetAAndFacetB.observe((value) => { + derivativeFacet.set(value) + }) + + derivativeFacetMapped.observe(observer) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('first a1_b1,second a1_b1') + + jest.clearAllMocks() + // this batch groups with the batch that's called as an effect of it's function call contents + batch(() => { + sourceFacetA.set('a2') + sourceFacetB.set('b2') + }) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('first a2_b2,second a2_b2') +}) diff --git a/packages/@react-facet/core/src/batch.ts b/packages/@react-facet/core/src/batch.ts new file mode 100644 index 00000000..54409d0c --- /dev/null +++ b/packages/@react-facet/core/src/batch.ts @@ -0,0 +1,31 @@ +export type Cb = () => void + +let batchId = -1 +let scheduledBatches = new Set() + +export const scheduleUpdate = (update: Cb) => { + if (batchId === -1) return false + scheduledBatches.add(update) + return true +} + +export const batch = (cb: Cb) => { + batchId += 1 + + cb() + + batchId -= 1 + + // We are back at the root batch call + if (batchId === -1) { + // Make a copy of the schedule + // As notifying can start other batch roots + const array = Array.from(scheduledBatches) + scheduledBatches = new Set() + + for (let index = array.length - 1; index >= 0; index--) { + const notify = array[index] + notify() + } + } +} diff --git a/packages/@react-facet/core/src/facet/createFacet.ts b/packages/@react-facet/core/src/facet/createFacet.ts index 8792f851..d0d611c3 100644 --- a/packages/@react-facet/core/src/facet/createFacet.ts +++ b/packages/@react-facet/core/src/facet/createFacet.ts @@ -1,5 +1,6 @@ import { defaultEqualityCheck } from '../equalityChecks' import { Cleanup, EqualityCheck, Listener, WritableFacet, StartSubscription, Option, NO_VALUE } from '../types' +import { batch } from '../batch' export interface FacetOptions { initialValue: Option @@ -21,32 +22,34 @@ export function createFacet({ const checker = equalityCheck?.() const update = (newValue: V) => { - if (equalityCheck != null) { - // we optimize for the most common scenario of using the defaultEqualityCheck (by inline its implementation) - if (equalityCheck === defaultEqualityCheck) { - const typeofValue = typeof newValue - if ( - (typeofValue === 'number' || - typeofValue === 'string' || - typeofValue === 'boolean' || - newValue === null || - newValue === undefined) && - currentValue === newValue - ) { - return - } - } else { - if (checker != null && checker(newValue)) { - return + batch(() => { + if (equalityCheck != null) { + // we optimize for the most common scenario of using the defaultEqualityCheck (by inline its implementation) + if (equalityCheck === defaultEqualityCheck) { + const typeofValue = typeof newValue + if ( + (typeofValue === 'number' || + typeofValue === 'string' || + typeofValue === 'boolean' || + newValue === null || + newValue === undefined) && + currentValue === newValue + ) { + return + } + } else { + if (checker != null && checker(newValue)) { + return + } } } - } - currentValue = newValue + currentValue = newValue - for (const listener of listeners) { - listener(currentValue) - } + for (const listener of listeners) { + listener(currentValue) + } + }) } /** diff --git a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.spec.ts b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.spec.ts index 76545658..6c10a079 100644 --- a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.spec.ts +++ b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.spec.ts @@ -1,9 +1,6 @@ import { defaultEqualityCheck } from '../equalityChecks' import { NO_VALUE } from '../types' -import { mapIntoObserveArray, batch } from './mapIntoObserveArray' -import { createFacet } from '../facet' -import { mapFacetSingleLightweight } from './mapFacetSingleLightweight' -import { mapFacetArrayLightweight } from './mapFacetArrayLightweight' +import { mapIntoObserveArray } from './mapIntoObserveArray' it('checks equality of primitives when passing defaultEqualityCheck', () => { const sourceA = { observe: jest.fn(), get: jest.fn() } @@ -229,107 +226,3 @@ describe('mapping to NO_VALUE', () => { expect(listener).not.toBeCalled() }) }) - -describe('batch', () => { - it('supports batching', () => { - const facetA = createFacet({ initialValue: 'a1' }) - const facetB = createFacet({ initialValue: 'b1' }) - const observe = mapIntoObserveArray([facetA, facetB], (a, b) => `${a} ${b}`) - - const observer = jest.fn() - observe(observer) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('a1 b1') - - jest.clearAllMocks() - batch(() => { - facetA.set('a2') - facetB.set('b2') - }) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('a2 b2') - }) - - it('supports batching, but nested', () => { - const facetA = createFacet({ initialValue: 'a1' }) - const facetB = createFacet({ initialValue: 'b1' }) - const observe = mapIntoObserveArray([facetA, facetB], (a, b) => `${a} ${b}`) - - const observer = jest.fn() - observe(observer) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('a1 b1') - - jest.clearAllMocks() - batch(() => { - facetA.set('a2') - batch(() => { - facetB.set('b2') - }) - }) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('a2 b2') - }) - - it('batches a single facet mapped into multiple facets and then combined again', () => { - const facet = createFacet({ initialValue: 'a' }) - const first = mapFacetSingleLightweight(facet, (a) => `first ${a}`) - const second = mapFacetSingleLightweight(facet, (a) => `second ${a}`) - const observe = mapIntoObserveArray([first, second], (a, b) => `${a},${b}`) - - const observer = jest.fn() - observe(observer) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('first a,second a') - - jest.clearAllMocks() - batch(() => { - facet.set('b') - }) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('first b,second b') - }) - - it('batches effects of other batches', () => { - const derivativeFacet = createFacet({ initialValue: 'a' }) - const derivativeFacetFirst = mapFacetSingleLightweight(derivativeFacet, (a) => `first ${a}`) - const derivativeFacetSecond = mapFacetSingleLightweight(derivativeFacet, (a) => `second ${a}`) - const derivativeFacetMapped = mapFacetArrayLightweight( - [derivativeFacetFirst, derivativeFacetSecond], - (a, b) => `${a},${b}`, - ) - - const sourceFacetA = createFacet({ initialValue: 'a1' }) - const sourceFacetB = createFacet({ initialValue: 'b1' }) - const mappedFacetAAndFacetB = mapFacetArrayLightweight([sourceFacetA, sourceFacetB], (a, b) => `${a}_${b}`) - - const observer = jest.fn() - mappedFacetAAndFacetB.observe((value) => { - batch(() => { - derivativeFacet.set(value) - }) - }) - - derivativeFacetMapped.observe(observer) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('first a1_b1,second a1_b1') - - jest.clearAllMocks() - // this batch groups with the batch that's called as an effect of it's function call contents - batch(() => { - sourceFacetA.set('a2') - sourceFacetB.set('b2') - console.log('got here') - }) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('first a2_b2,second a2_b2') - }) -}) diff --git a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts index 272f59a1..63ee28bd 100644 --- a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts +++ b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts @@ -1,3 +1,4 @@ +import { scheduleUpdate } from '../batch' import { defaultEqualityCheck } from '../equalityChecks' import { EqualityCheck, Listener, Option, NO_VALUE, Observe, Facet, NoValue } from '../types' @@ -14,38 +15,22 @@ export function mapIntoObserveArray( const dependencyValues: Option[] = facets.map(() => NO_VALUE) let hasAllDependencies = false - const notify = () => { - const result = fn(...dependencyValues) - if (result === NO_VALUE) return + const notify = + checker == null + ? () => { + hasAllDependencies = hasAllDependencies || dependencyValues.every((value) => value != NO_VALUE) + if (!hasAllDependencies) return - listener(result) - } - - const subscriptions = facets.map((facet, index) => { - // Most common scenario is not having any equality check - if (equalityCheck == null) { - return facet.observe((value) => { - dependencyValues[index] = value + const result = fn(...dependencyValues) + if (result === NO_VALUE) return - if (batchId >= 0) { - scheduledBatches.add(notify) - return + listener(result) } + : equalityCheck === defaultEqualityCheck + ? () => { + hasAllDependencies = hasAllDependencies || dependencyValues.every((value) => value != NO_VALUE) + if (!hasAllDependencies) return - hasAllDependencies = hasAllDependencies || dependencyValues.every((value) => value != NO_VALUE) - - if (hasAllDependencies) notify() - }) - } - - // Then we optimize for the second most common scenario of using the defaultEqualityCheck (by inline its implementation) - if (equalityCheck === defaultEqualityCheck) { - return facet.observe((value) => { - dependencyValues[index] = value - - hasAllDependencies = hasAllDependencies || dependencyValues.every((value) => value != NO_VALUE) - - if (hasAllDependencies) { const result = fn(...dependencyValues) if (result === NO_VALUE) return @@ -66,25 +51,21 @@ export function mapIntoObserveArray( listener(result) } - }) - } + : () => { + hasAllDependencies = hasAllDependencies || dependencyValues.every((value) => value != NO_VALUE) + if (!hasAllDependencies) return - // Just a type-check guard, it will never happen - if (checker == null) return () => {} + const result = fn(...dependencyValues) + if (result === NO_VALUE) return + if (checker(result)) return + + listener(result) + } - // Finally we use the custom equality check + const subscriptions = facets.map((facet, index) => { return facet.observe((value) => { dependencyValues[index] = value - - hasAllDependencies = hasAllDependencies || dependencyValues.every((value) => value != NO_VALUE) - - if (hasAllDependencies) { - const result = fn(...dependencyValues) - if (result === NO_VALUE) return - if (checker(result)) return - - listener(result) - } + if (scheduleUpdate(notify) != true) notify() }) }) @@ -93,29 +74,3 @@ export function mapIntoObserveArray( } } } - -export type Cb = () => void - -let batchId = -1 -let scheduledBatches = new Set() - -export const batch = (cb: Cb) => { - batchId += 1 - - cb() - - batchId -= 1 - - // We are back at the root batch call - if (batchId === -1) { - // Make a copy of the schedule - // As notifying can start other batch roots - const array = Array.from(scheduledBatches) - scheduledBatches = new Set() - - for (let index = array.length - 1; index >= 0; index--) { - const notify = array[index] - notify() - } - } -} From cfc99470c59db2be60547983728153ecd4e7b76a Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Wed, 7 Jun 2023 15:46:24 +0200 Subject: [PATCH 04/23] WIP: logging extra usage --- packages/@react-facet/core/src/batch.ts | 40 +++++++++++++++++-------- 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/packages/@react-facet/core/src/batch.ts b/packages/@react-facet/core/src/batch.ts index 54409d0c..1ec5b779 100644 --- a/packages/@react-facet/core/src/batch.ts +++ b/packages/@react-facet/core/src/batch.ts @@ -1,15 +1,24 @@ -export type Cb = () => void +export type Task = () => void -let batchId = -1 -let scheduledBatches = new Set() +let batchId = 0 +let scheduledBatches = new Set() +const taskCounter = new Map() -export const scheduleUpdate = (update: Cb) => { - if (batchId === -1) return false - scheduledBatches.add(update) +export const scheduleUpdate = (task: Task) => { + if (batchId === 0) return false + + if (scheduledBatches.has(task)) { + console.log('⚠️This would execute twice on this frame!') + } + + const currentCount = taskCounter.get(task) ?? 0 + taskCounter.set(task, currentCount + 1) + + scheduledBatches.add(task) return true } -export const batch = (cb: Cb) => { +export const batch = (cb: Task) => { batchId += 1 cb() @@ -17,15 +26,22 @@ export const batch = (cb: Cb) => { batchId -= 1 // We are back at the root batch call - if (batchId === -1) { + if (batchId === 0) { + const taskCounterCopy = Array.from(taskCounter) + taskCounter.clear() + + const optimizedCount = taskCounterCopy.filter(([_, count]) => count > 1).length + if (taskCounterCopy.length > 0) { + console.log(`⚒️ Total: ${taskCounterCopy.length}. Optimized: ${optimizedCount}`) + } + // Make a copy of the schedule // As notifying can start other batch roots const array = Array.from(scheduledBatches) - scheduledBatches = new Set() + scheduledBatches = new Set() - for (let index = array.length - 1; index >= 0; index--) { - const notify = array[index] - notify() + for (const task of array) { + task() } } } From 876e796375f9f9d777662004bff257283a8cd038 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Wed, 7 Jun 2023 15:56:14 +0200 Subject: [PATCH 05/23] Remove log and schedule on effects --- packages/@react-facet/core/src/batch.ts | 20 +++------------- .../core/src/hooks/useFacetEffect.tsx | 23 +++++++++++-------- .../core/src/mapFacets/mapIntoObserveArray.ts | 2 +- 3 files changed, 18 insertions(+), 27 deletions(-) diff --git a/packages/@react-facet/core/src/batch.ts b/packages/@react-facet/core/src/batch.ts index 1ec5b779..47ac6589 100644 --- a/packages/@react-facet/core/src/batch.ts +++ b/packages/@react-facet/core/src/batch.ts @@ -2,20 +2,14 @@ export type Task = () => void let batchId = 0 let scheduledBatches = new Set() -const taskCounter = new Map() export const scheduleUpdate = (task: Task) => { - if (batchId === 0) return false - - if (scheduledBatches.has(task)) { - console.log('⚠️This would execute twice on this frame!') + if (batchId === 0) { + task() + return } - const currentCount = taskCounter.get(task) ?? 0 - taskCounter.set(task, currentCount + 1) - scheduledBatches.add(task) - return true } export const batch = (cb: Task) => { @@ -27,14 +21,6 @@ export const batch = (cb: Task) => { // We are back at the root batch call if (batchId === 0) { - const taskCounterCopy = Array.from(taskCounter) - taskCounter.clear() - - const optimizedCount = taskCounterCopy.filter(([_, count]) => count > 1).length - if (taskCounterCopy.length > 0) { - console.log(`⚒️ Total: ${taskCounterCopy.length}. Optimized: ${optimizedCount}`) - } - // Make a copy of the schedule // As notifying can start other batch roots const array = Array.from(scheduledBatches) diff --git a/packages/@react-facet/core/src/hooks/useFacetEffect.tsx b/packages/@react-facet/core/src/hooks/useFacetEffect.tsx index 0decd08a..1602b59c 100644 --- a/packages/@react-facet/core/src/hooks/useFacetEffect.tsx +++ b/packages/@react-facet/core/src/hooks/useFacetEffect.tsx @@ -1,5 +1,6 @@ import { useCallback, useEffect, useLayoutEffect } from 'react' import { Facet, Unsubscribe, Cleanup, NO_VALUE, ExtractFacetValues } from '../types' +import { scheduleUpdate } from '../batch' export const createUseFacetEffect = (useHook: typeof useEffect | typeof useLayoutEffect) => { return function [], T extends [...Y]>( @@ -34,18 +35,22 @@ export const createUseFacetEffect = (useHook: typeof useEffect | typeof useLayou const unsubscribes: Unsubscribe[] = [] const values: unknown[] = facets.map(() => NO_VALUE) + const task = () => { + hasAllDependencies = hasAllDependencies || values.every((value) => value != NO_VALUE) + + if (hasAllDependencies) { + if (cleanup != null) { + cleanup() + } + + cleanup = effectMemoized(...(values as ExtractFacetValues)) + } + } + facets.forEach((facet, index) => { unsubscribes[index] = facet.observe((value) => { values[index] = value - hasAllDependencies = hasAllDependencies || values.every((value) => value != NO_VALUE) - - if (hasAllDependencies) { - if (cleanup != null) { - cleanup() - } - - cleanup = effectMemoized(...(values as ExtractFacetValues)) - } + scheduleUpdate(task) }) }) diff --git a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts index 63ee28bd..9ad5e0a5 100644 --- a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts +++ b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts @@ -65,7 +65,7 @@ export function mapIntoObserveArray( const subscriptions = facets.map((facet, index) => { return facet.observe((value) => { dependencyValues[index] = value - if (scheduleUpdate(notify) != true) notify() + scheduleUpdate(notify) }) }) From 0290201b9daddde718d33eb7c540472d98e6e81b Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Wed, 7 Jun 2023 15:58:24 +0200 Subject: [PATCH 06/23] Logs scheduler success ration --- packages/@react-facet/core/src/batch.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/@react-facet/core/src/batch.ts b/packages/@react-facet/core/src/batch.ts index 47ac6589..32e14be4 100644 --- a/packages/@react-facet/core/src/batch.ts +++ b/packages/@react-facet/core/src/batch.ts @@ -2,6 +2,7 @@ export type Task = () => void let batchId = 0 let scheduledBatches = new Set() +const taskCounter = new Map() export const scheduleUpdate = (task: Task) => { if (batchId === 0) { @@ -9,6 +10,11 @@ export const scheduleUpdate = (task: Task) => { return } + if (process.env.NODE_ENV === 'development') { + const currentCount = taskCounter.get(task) ?? 0 + taskCounter.set(task, currentCount + 1) + } + scheduledBatches.add(task) } @@ -21,6 +27,16 @@ export const batch = (cb: Task) => { // We are back at the root batch call if (batchId === 0) { + if (process.env.NODE_ENV === 'development') { + const taskCounterCopy = Array.from(taskCounter) + taskCounter.clear() + + const optimizedCount = taskCounterCopy.filter(([_, count]) => count > 1).length + if (taskCounterCopy.length > 0) { + console.log(`⚒️ Total: ${taskCounterCopy.length}. Optimized: ${optimizedCount}`) + } + } + // Make a copy of the schedule // As notifying can start other batch roots const array = Array.from(scheduledBatches) From 0b02ec751e7d3594c6117345d0830d07717cfcee Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 8 Jun 2023 08:58:31 +0200 Subject: [PATCH 07/23] Rename batch into scheduler --- packages/@react-facet/core/src/facet/createFacet.ts | 2 +- .../@react-facet/core/src/hooks/useFacetEffect.tsx | 4 ++-- .../core/src/mapFacets/mapIntoObserveArray.ts | 6 +++--- .../core/src/{batch.spec.ts => scheduler.spec.ts} | 2 +- .../@react-facet/core/src/{batch.ts => scheduler.ts} | 12 ++++++------ packages/@react-facet/core/src/types.ts | 2 ++ 6 files changed, 15 insertions(+), 13 deletions(-) rename packages/@react-facet/core/src/{batch.spec.ts => scheduler.spec.ts} (98%) rename packages/@react-facet/core/src/{batch.ts => scheduler.ts} (79%) diff --git a/packages/@react-facet/core/src/facet/createFacet.ts b/packages/@react-facet/core/src/facet/createFacet.ts index d0d611c3..73da8e39 100644 --- a/packages/@react-facet/core/src/facet/createFacet.ts +++ b/packages/@react-facet/core/src/facet/createFacet.ts @@ -1,6 +1,6 @@ import { defaultEqualityCheck } from '../equalityChecks' import { Cleanup, EqualityCheck, Listener, WritableFacet, StartSubscription, Option, NO_VALUE } from '../types' -import { batch } from '../batch' +import { batch } from '../scheduler' export interface FacetOptions { initialValue: Option diff --git a/packages/@react-facet/core/src/hooks/useFacetEffect.tsx b/packages/@react-facet/core/src/hooks/useFacetEffect.tsx index 1602b59c..0660be80 100644 --- a/packages/@react-facet/core/src/hooks/useFacetEffect.tsx +++ b/packages/@react-facet/core/src/hooks/useFacetEffect.tsx @@ -1,6 +1,6 @@ import { useCallback, useEffect, useLayoutEffect } from 'react' import { Facet, Unsubscribe, Cleanup, NO_VALUE, ExtractFacetValues } from '../types' -import { scheduleUpdate } from '../batch' +import { scheduleTask } from '../scheduler' export const createUseFacetEffect = (useHook: typeof useEffect | typeof useLayoutEffect) => { return function [], T extends [...Y]>( @@ -50,7 +50,7 @@ export const createUseFacetEffect = (useHook: typeof useEffect | typeof useLayou facets.forEach((facet, index) => { unsubscribes[index] = facet.observe((value) => { values[index] = value - scheduleUpdate(task) + scheduleTask(task) }) }) diff --git a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts index 9ad5e0a5..43931f9d 100644 --- a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts +++ b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts @@ -1,4 +1,4 @@ -import { scheduleUpdate } from '../batch' +import { scheduleTask } from '../scheduler' import { defaultEqualityCheck } from '../equalityChecks' import { EqualityCheck, Listener, Option, NO_VALUE, Observe, Facet, NoValue } from '../types' @@ -15,7 +15,7 @@ export function mapIntoObserveArray( const dependencyValues: Option[] = facets.map(() => NO_VALUE) let hasAllDependencies = false - const notify = + const task = checker == null ? () => { hasAllDependencies = hasAllDependencies || dependencyValues.every((value) => value != NO_VALUE) @@ -65,7 +65,7 @@ export function mapIntoObserveArray( const subscriptions = facets.map((facet, index) => { return facet.observe((value) => { dependencyValues[index] = value - scheduleUpdate(notify) + scheduleTask(task) }) }) diff --git a/packages/@react-facet/core/src/batch.spec.ts b/packages/@react-facet/core/src/scheduler.spec.ts similarity index 98% rename from packages/@react-facet/core/src/batch.spec.ts rename to packages/@react-facet/core/src/scheduler.spec.ts index 2246ee74..9ce5e552 100644 --- a/packages/@react-facet/core/src/batch.spec.ts +++ b/packages/@react-facet/core/src/scheduler.spec.ts @@ -1,6 +1,6 @@ import { createFacet } from './facet' import { mapFacetsLightweight } from './mapFacets' -import { batch } from './batch' +import { batch } from './scheduler' it('supports batching', () => { const facetA = createFacet({ initialValue: 'a1' }) diff --git a/packages/@react-facet/core/src/batch.ts b/packages/@react-facet/core/src/scheduler.ts similarity index 79% rename from packages/@react-facet/core/src/batch.ts rename to packages/@react-facet/core/src/scheduler.ts index 32e14be4..99bd3913 100644 --- a/packages/@react-facet/core/src/batch.ts +++ b/packages/@react-facet/core/src/scheduler.ts @@ -1,10 +1,10 @@ -export type Task = () => void +import { Task } from './types' let batchId = 0 -let scheduledBatches = new Set() +const scheduledTasks = new Set() const taskCounter = new Map() -export const scheduleUpdate = (task: Task) => { +export const scheduleTask = (task: Task) => { if (batchId === 0) { task() return @@ -15,7 +15,7 @@ export const scheduleUpdate = (task: Task) => { taskCounter.set(task, currentCount + 1) } - scheduledBatches.add(task) + scheduledTasks.add(task) } export const batch = (cb: Task) => { @@ -39,8 +39,8 @@ export const batch = (cb: Task) => { // Make a copy of the schedule // As notifying can start other batch roots - const array = Array.from(scheduledBatches) - scheduledBatches = new Set() + const array = Array.from(scheduledTasks) + scheduledTasks.clear() for (const task of array) { task() diff --git a/packages/@react-facet/core/src/types.ts b/packages/@react-facet/core/src/types.ts index edd046ff..2068df54 100644 --- a/packages/@react-facet/core/src/types.ts +++ b/packages/@react-facet/core/src/types.ts @@ -71,6 +71,8 @@ export type NoValue = typeof NO_VALUE export type Option = NoValue | T +export type Task = () => void + /** * CSSStyleDeclaration that has FacetDefinition as values and includes Gameface's extended number based properties * Based on Gameface's TypeScript definition: https://github.com/Mojang/Mojang_Cohtml/blob/master/src/Scripting/scripted/typescript/ScriptedCSSStyleDeclaration.d.ts From 131e09fcaecfc0778f0128f6016101a1de32daca Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 8 Jun 2023 09:05:36 +0200 Subject: [PATCH 08/23] Documentation --- packages/@react-facet/core/src/scheduler.ts | 15 ++++++++++++--- packages/@react-facet/core/src/types.ts | 8 ++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/packages/@react-facet/core/src/scheduler.ts b/packages/@react-facet/core/src/scheduler.ts index 99bd3913..d3332468 100644 --- a/packages/@react-facet/core/src/scheduler.ts +++ b/packages/@react-facet/core/src/scheduler.ts @@ -1,9 +1,14 @@ -import { Task } from './types' +import { Task, Batch } from './types' let batchId = 0 const scheduledTasks = new Set() const taskCounter = new Map() +/** + * Schedules a given task to be executed at the end of a current batch, or runs it immediately if no batch is active. + * @param task + * @returns + */ export const scheduleTask = (task: Task) => { if (batchId === 0) { task() @@ -18,10 +23,14 @@ export const scheduleTask = (task: Task) => { scheduledTasks.add(task) } -export const batch = (cb: Task) => { +/** + * Starts a batch, scheduling Facet updates within the cb to be executed at the end of the batch. + * @param b will be executed immediately to collect Facet changes + */ +export const batch = (b: Batch) => { batchId += 1 - cb() + b() batchId -= 1 diff --git a/packages/@react-facet/core/src/types.ts b/packages/@react-facet/core/src/types.ts index 2068df54..4e0b01a6 100644 --- a/packages/@react-facet/core/src/types.ts +++ b/packages/@react-facet/core/src/types.ts @@ -71,8 +71,16 @@ export type NoValue = typeof NO_VALUE export type Option = NoValue | T +/** + * Unit of work that can be scheduled within a batch. + */ export type Task = () => void +/** + * Function that when executed will have its Facet updates scheduled. + */ +export type Batch = () => void + /** * CSSStyleDeclaration that has FacetDefinition as values and includes Gameface's extended number based properties * Based on Gameface's TypeScript definition: https://github.com/Mojang/Mojang_Cohtml/blob/master/src/Scripting/scripted/typescript/ScriptedCSSStyleDeclaration.d.ts From 72133f697351a30b8a21dd74a23efffde2fbbe2f Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 8 Jun 2023 09:33:19 +0200 Subject: [PATCH 09/23] Only starts a batch on createFacet if we are notifying a change --- .../core/src/facet/createFacet.ts | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/packages/@react-facet/core/src/facet/createFacet.ts b/packages/@react-facet/core/src/facet/createFacet.ts index 73da8e39..bf397a3f 100644 --- a/packages/@react-facet/core/src/facet/createFacet.ts +++ b/packages/@react-facet/core/src/facet/createFacet.ts @@ -22,28 +22,28 @@ export function createFacet({ const checker = equalityCheck?.() const update = (newValue: V) => { - batch(() => { - if (equalityCheck != null) { - // we optimize for the most common scenario of using the defaultEqualityCheck (by inline its implementation) - if (equalityCheck === defaultEqualityCheck) { - const typeofValue = typeof newValue - if ( - (typeofValue === 'number' || - typeofValue === 'string' || - typeofValue === 'boolean' || - newValue === null || - newValue === undefined) && - currentValue === newValue - ) { - return - } - } else { - if (checker != null && checker(newValue)) { - return - } + if (equalityCheck != null) { + // we optimize for the most common scenario of using the defaultEqualityCheck (by inline its implementation) + if (equalityCheck === defaultEqualityCheck) { + const typeofValue = typeof newValue + if ( + (typeofValue === 'number' || + typeofValue === 'string' || + typeofValue === 'boolean' || + newValue === null || + newValue === undefined) && + currentValue === newValue + ) { + return + } + } else { + if (checker != null && checker(newValue)) { + return } } + } + batch(() => { currentValue = newValue for (const listener of listeners) { From 642c8f245051cfeb18ae9449d40156327cfc5a79 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 8 Jun 2023 09:42:59 +0200 Subject: [PATCH 10/23] Also test useFacetEffect --- .../@react-facet/core/src/scheduler.spec.ts | 100 ------------ .../@react-facet/core/src/scheduler.spec.tsx | 154 ++++++++++++++++++ 2 files changed, 154 insertions(+), 100 deletions(-) delete mode 100644 packages/@react-facet/core/src/scheduler.spec.ts create mode 100644 packages/@react-facet/core/src/scheduler.spec.tsx diff --git a/packages/@react-facet/core/src/scheduler.spec.ts b/packages/@react-facet/core/src/scheduler.spec.ts deleted file mode 100644 index 9ce5e552..00000000 --- a/packages/@react-facet/core/src/scheduler.spec.ts +++ /dev/null @@ -1,100 +0,0 @@ -import { createFacet } from './facet' -import { mapFacetsLightweight } from './mapFacets' -import { batch } from './scheduler' - -it('supports batching', () => { - const facetA = createFacet({ initialValue: 'a1' }) - const facetB = createFacet({ initialValue: 'b1' }) - const facetAB = mapFacetsLightweight([facetA, facetB], (a, b) => `${a} ${b}`) - - const observer = jest.fn() - facetAB.observe(observer) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('a1 b1') - - jest.clearAllMocks() - batch(() => { - facetA.set('a2') - facetB.set('b2') - }) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('a2 b2') -}) - -it('supports batching, but nested', () => { - const facetA = createFacet({ initialValue: 'a1' }) - const facetB = createFacet({ initialValue: 'b1' }) - const facetAB = mapFacetsLightweight([facetA, facetB], (a, b) => `${a} ${b}`) - - const observer = jest.fn() - facetAB.observe(observer) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('a1 b1') - - jest.clearAllMocks() - batch(() => { - facetA.set('a2') - batch(() => { - facetB.set('b2') - }) - }) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('a2 b2') -}) - -it('batches a single facet mapped into multiple facets and then combined again', () => { - const facet = createFacet({ initialValue: 'a' }) - const first = mapFacetsLightweight([facet], (a) => `first ${a}`) - const second = mapFacetsLightweight([facet], (a) => `second ${a}`) - const combinedAgain = mapFacetsLightweight([first, second], (a, b) => `${a},${b}`) - - const observer = jest.fn() - combinedAgain.observe(observer) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('first a,second a') - - jest.clearAllMocks() - facet.set('b') - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('first b,second b') -}) - -it('batches effects of other batches', () => { - const derivativeFacet = createFacet({ initialValue: 'a' }) - const derivativeFacetFirst = mapFacetsLightweight([derivativeFacet], (a) => `first ${a}`) - const derivativeFacetSecond = mapFacetsLightweight([derivativeFacet], (a) => `second ${a}`) - const derivativeFacetMapped = mapFacetsLightweight( - [derivativeFacetFirst, derivativeFacetSecond], - (a, b) => `${a},${b}`, - ) - - const sourceFacetA = createFacet({ initialValue: 'a1' }) - const sourceFacetB = createFacet({ initialValue: 'b1' }) - const mappedFacetAAndFacetB = mapFacetsLightweight([sourceFacetA, sourceFacetB], (a, b) => `${a}_${b}`) - - const observer = jest.fn() - mappedFacetAAndFacetB.observe((value) => { - derivativeFacet.set(value) - }) - - derivativeFacetMapped.observe(observer) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('first a1_b1,second a1_b1') - - jest.clearAllMocks() - // this batch groups with the batch that's called as an effect of it's function call contents - batch(() => { - sourceFacetA.set('a2') - sourceFacetB.set('b2') - }) - - expect(observer).toHaveBeenCalledTimes(1) - expect(observer).toHaveBeenCalledWith('first a2_b2,second a2_b2') -}) diff --git a/packages/@react-facet/core/src/scheduler.spec.tsx b/packages/@react-facet/core/src/scheduler.spec.tsx new file mode 100644 index 00000000..03b20856 --- /dev/null +++ b/packages/@react-facet/core/src/scheduler.spec.tsx @@ -0,0 +1,154 @@ +import React from 'react' +import { createFacet } from './facet' +import { useFacetEffect } from './hooks' +import { mapFacetsLightweight } from './mapFacets' +import { batch } from './scheduler' +import { act, render } from '@react-facet/dom-fiber-testing-library' + +describe('mapping an array of facets', () => { + it('supports batching', () => { + const facetA = createFacet({ initialValue: 'a1' }) + const facetB = createFacet({ initialValue: 'b1' }) + const facetAB = mapFacetsLightweight([facetA, facetB], (a, b) => `${a} ${b}`) + + const observer = jest.fn() + facetAB.observe(observer) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('a1 b1') + + jest.clearAllMocks() + batch(() => { + facetA.set('a2') + facetB.set('b2') + }) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('a2 b2') + }) + + it('supports batching, but nested', () => { + const facetA = createFacet({ initialValue: 'a1' }) + const facetB = createFacet({ initialValue: 'b1' }) + const facetAB = mapFacetsLightweight([facetA, facetB], (a, b) => `${a} ${b}`) + + const observer = jest.fn() + facetAB.observe(observer) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('a1 b1') + + jest.clearAllMocks() + batch(() => { + facetA.set('a2') + batch(() => { + facetB.set('b2') + }) + }) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('a2 b2') + }) + + it('batches a single facet mapped into multiple facets and then combined again', () => { + const facet = createFacet({ initialValue: 'a' }) + const first = mapFacetsLightweight([facet], (a) => `first ${a}`) + const second = mapFacetsLightweight([facet], (a) => `second ${a}`) + const combinedAgain = mapFacetsLightweight([first, second], (a, b) => `${a},${b}`) + + const observer = jest.fn() + combinedAgain.observe(observer) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('first a,second a') + + jest.clearAllMocks() + facet.set('b') + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('first b,second b') + }) + + it('batches side-effects of other batches', () => { + const derivativeFacet = createFacet({ initialValue: 'a' }) + const derivativeFacetFirst = mapFacetsLightweight([derivativeFacet], (a) => `first ${a}`) + const derivativeFacetSecond = mapFacetsLightweight([derivativeFacet], (a) => `second ${a}`) + const derivativeFacetMapped = mapFacetsLightweight( + [derivativeFacetFirst, derivativeFacetSecond], + (a, b) => `${a},${b}`, + ) + + const sourceFacetA = createFacet({ initialValue: 'a1' }) + const sourceFacetB = createFacet({ initialValue: 'b1' }) + const mappedFacetAAndFacetB = mapFacetsLightweight([sourceFacetA, sourceFacetB], (a, b) => `${a}_${b}`) + + const observer = jest.fn() + mappedFacetAAndFacetB.observe((value) => { + derivativeFacet.set(value) + }) + + derivativeFacetMapped.observe(observer) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('first a1_b1,second a1_b1') + + jest.clearAllMocks() + // this batch groups with the batch that's called as an effect of it's function call contents + batch(() => { + sourceFacetA.set('a2') + sourceFacetB.set('b2') + }) + + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('first a2_b2,second a2_b2') + }) +}) + +describe('effects with multiple facet dependencies', () => { + it('batches dependencies of an effect', () => { + const facetA = createFacet({ initialValue: 'facetA' }) + const facetB = createFacet({ initialValue: 'facetB' }) + + const cleanup = jest.fn() + const effect = jest.fn().mockReturnValue(cleanup) + + const ComponentWithFacetEffect = () => { + useFacetEffect(effect, [], [facetA, facetB]) + + return null + } + + const scenario = + + const { rerender } = render(scenario) + + expect(cleanup).not.toHaveBeenCalled() + expect(effect).toHaveBeenCalledWith('facetA', 'facetB') + expect(effect).toHaveBeenCalledTimes(1) + effect.mockClear() + cleanup.mockClear() + + // set a value for the second facet + act(() => { + batch(() => { + facetA.set('facetA-updated') + facetB.set('facetB-updated') + }) + }) + + // Runs the previous cleanup, then runs the effect only once + expect(cleanup).toHaveBeenCalled() + expect(cleanup).toHaveBeenCalledTimes(1) + expect(effect).toHaveBeenCalledWith('facetA-updated', 'facetB-updated') + expect(effect).toHaveBeenCalledTimes(1) + effect.mockClear() + cleanup.mockClear() + + // and finally we unmount the component + rerender(<>) + + // then we get a final cleanup, without the effect being fired + expect(cleanup).toHaveBeenCalledTimes(1) + expect(effect).not.toHaveBeenCalled() + }) +}) From 706e8454d78850b8d4222d7af6449560815d19b5 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 8 Jun 2023 10:18:28 +0200 Subject: [PATCH 11/23] Keep batch started until the end Making sure to exhaust all tasks before exiting --- .../@react-facet/core/src/scheduler.spec.tsx | 51 ++++++++++++++++++- packages/@react-facet/core/src/scheduler.ts | 28 ++++++---- 2 files changed, 68 insertions(+), 11 deletions(-) diff --git a/packages/@react-facet/core/src/scheduler.spec.tsx b/packages/@react-facet/core/src/scheduler.spec.tsx index 03b20856..b26ec738 100644 --- a/packages/@react-facet/core/src/scheduler.spec.tsx +++ b/packages/@react-facet/core/src/scheduler.spec.tsx @@ -1,10 +1,59 @@ import React from 'react' import { createFacet } from './facet' -import { useFacetEffect } from './hooks' +import { useFacetEffect, useFacetMap } from './hooks' import { mapFacetsLightweight } from './mapFacets' import { batch } from './scheduler' import { act, render } from '@react-facet/dom-fiber-testing-library' +/** + * Integration test to demonstrate the main motivation for batching and scheduling + */ +it('batches maps and effects', () => { + type TestData = { name: string; login: string } + const friendsCountFacet = createFacet({ initialValue: 42 }) + const userFacet = createFacet({ initialValue: { name: 'Initial Name', login: 'Initial Login' } }) + + const cleanup = jest.fn() + const effect = jest.fn().mockReturnValue(cleanup) + + // The useFacetMaps in the component below are within a single component, + // but more realistically you can think that they would be distributed across a React tree. + const ComponentWithFacetEffect = () => { + // Its not unusual to want to combine data from multiple Facet sources + const userWithFriends = useFacetMap( + (user, friendsCount) => ({ ...user, friendsCount }), + [], + [userFacet, friendsCountFacet], + ) + + // Another very common scenario with Facets is that we can map them into more specific values + const nameFacet = useFacetMap(({ name }) => name, [], [userWithFriends]) + const loginFacet = useFacetMap(({ login }) => login, [], [userWithFriends]) + + // But then we might decide again on combining both on a single effect + useFacetEffect(effect, [], [nameFacet, loginFacet]) + + return null + } + + const scenario = + render(scenario) + effect.mockClear() + cleanup.mockClear() + + act(() => { + // On updating the facet, we should expect that the effect is called only once + // Without batching, it would have been called twice + batch(() => { + userFacet.set({ name: 'New Name', login: 'New Login' }) + }) + }) + + expect(cleanup).toHaveBeenCalledTimes(1) + expect(effect).toHaveBeenCalledWith('New Name', 'New Login') + expect(effect).toHaveBeenCalledTimes(1) +}) + describe('mapping an array of facets', () => { it('supports batching', () => { const facetA = createFacet({ initialValue: 'a1' }) diff --git a/packages/@react-facet/core/src/scheduler.ts b/packages/@react-facet/core/src/scheduler.ts index d3332468..7bfb7aef 100644 --- a/packages/@react-facet/core/src/scheduler.ts +++ b/packages/@react-facet/core/src/scheduler.ts @@ -28,14 +28,13 @@ export const scheduleTask = (task: Task) => { * @param b will be executed immediately to collect Facet changes */ export const batch = (b: Batch) => { + // Starts a batch batchId += 1 b() - batchId -= 1 - - // We are back at the root batch call - if (batchId === 0) { + // If this is the root batch, we start executing the tasks + if (batchId === 1) { if (process.env.NODE_ENV === 'development') { const taskCounterCopy = Array.from(taskCounter) taskCounter.clear() @@ -46,13 +45,22 @@ export const batch = (b: Batch) => { } } - // Make a copy of the schedule - // As notifying can start other batch roots - const array = Array.from(scheduledTasks) - scheduledTasks.clear() + // Exhaust tasks + let hasTasks = true + while (hasTasks) { + // Make a copy of the schedule + // As notifying can start other batch roots + const array = Array.from(scheduledTasks) + scheduledTasks.clear() + + hasTasks = array.length > 0 - for (const task of array) { - task() + for (const task of array) { + task() + } } } + + // Ends a batch + batchId -= 1 } From 223368239ba81b34139ab8c50d3e8f9907bf8e03 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 8 Jun 2023 10:25:17 +0200 Subject: [PATCH 12/23] Fix linter --- packages/@react-facet/core/src/scheduler.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@react-facet/core/src/scheduler.ts b/packages/@react-facet/core/src/scheduler.ts index 7bfb7aef..8a9ecb94 100644 --- a/packages/@react-facet/core/src/scheduler.ts +++ b/packages/@react-facet/core/src/scheduler.ts @@ -39,7 +39,7 @@ export const batch = (b: Batch) => { const taskCounterCopy = Array.from(taskCounter) taskCounter.clear() - const optimizedCount = taskCounterCopy.filter(([_, count]) => count > 1).length + const optimizedCount = taskCounterCopy.filter((pair) => pair[1] > 1).length if (taskCounterCopy.length > 0) { console.log(`⚒️ Total: ${taskCounterCopy.length}. Optimized: ${optimizedCount}`) } From f27c5215a3b8b15071bf016871c264ba3ef65d2a Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 8 Jun 2023 10:32:29 +0200 Subject: [PATCH 13/23] Expose batch as a public API --- packages/@react-facet/core/src/index.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@react-facet/core/src/index.ts b/packages/@react-facet/core/src/index.ts index 7a668bea..7ad486d0 100644 --- a/packages/@react-facet/core/src/index.ts +++ b/packages/@react-facet/core/src/index.ts @@ -7,3 +7,4 @@ export * from './helpers' export * from './hooks' export * from './mapFacets' export * from './types' +export { batch } from './scheduler' From fb55b16ab89cca98a346797ab17847b7d0423956 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 8 Jun 2023 11:02:44 +0200 Subject: [PATCH 14/23] Small refactor --- packages/@react-facet/core/src/scheduler.ts | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/packages/@react-facet/core/src/scheduler.ts b/packages/@react-facet/core/src/scheduler.ts index 8a9ecb94..e070f0a3 100644 --- a/packages/@react-facet/core/src/scheduler.ts +++ b/packages/@react-facet/core/src/scheduler.ts @@ -45,20 +45,17 @@ export const batch = (b: Batch) => { } } - // Exhaust tasks - let hasTasks = true - while (hasTasks) { - // Make a copy of the schedule - // As notifying can start other batch roots - const array = Array.from(scheduledTasks) + do { + // Make a copy of the schedule, as running a task can cause other tasks to be scheduled + const copiedScheduledTasks = Array.from(scheduledTasks) scheduledTasks.clear() - hasTasks = array.length > 0 - - for (const task of array) { + for (const task of copiedScheduledTasks) { task() } - } + + // Exhaust all tasks + } while (scheduledTasks.size > 0) } // Ends a batch From 509fb142fd9325f290ef6b81fcbca80d8c990955 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 8 Jun 2023 13:18:27 +0200 Subject: [PATCH 15/23] Make sure to cancel tasks that are no longer needed Needs unit tests --- packages/@react-facet/core/src/hooks/useFacetEffect.tsx | 3 ++- .../@react-facet/core/src/mapFacets/mapIntoObserveArray.ts | 3 ++- packages/@react-facet/core/src/scheduler.ts | 4 ++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/@react-facet/core/src/hooks/useFacetEffect.tsx b/packages/@react-facet/core/src/hooks/useFacetEffect.tsx index 0660be80..80ee3513 100644 --- a/packages/@react-facet/core/src/hooks/useFacetEffect.tsx +++ b/packages/@react-facet/core/src/hooks/useFacetEffect.tsx @@ -1,6 +1,6 @@ import { useCallback, useEffect, useLayoutEffect } from 'react' import { Facet, Unsubscribe, Cleanup, NO_VALUE, ExtractFacetValues } from '../types' -import { scheduleTask } from '../scheduler' +import { cancelScheduledTask, scheduleTask } from '../scheduler' export const createUseFacetEffect = (useHook: typeof useEffect | typeof useLayoutEffect) => { return function [], T extends [...Y]>( @@ -55,6 +55,7 @@ export const createUseFacetEffect = (useHook: typeof useEffect | typeof useLayou }) return () => { + cancelScheduledTask(task) unsubscribes.forEach((unsubscribe) => unsubscribe()) if (cleanup != null) { cleanup() diff --git a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts index 43931f9d..2f729b4a 100644 --- a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts +++ b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts @@ -1,4 +1,4 @@ -import { scheduleTask } from '../scheduler' +import { cancelScheduledTask, scheduleTask } from '../scheduler' import { defaultEqualityCheck } from '../equalityChecks' import { EqualityCheck, Listener, Option, NO_VALUE, Observe, Facet, NoValue } from '../types' @@ -70,6 +70,7 @@ export function mapIntoObserveArray( }) return () => { + cancelScheduledTask(task) subscriptions.forEach((unsubscribe) => unsubscribe()) } } diff --git a/packages/@react-facet/core/src/scheduler.ts b/packages/@react-facet/core/src/scheduler.ts index e070f0a3..a13fe198 100644 --- a/packages/@react-facet/core/src/scheduler.ts +++ b/packages/@react-facet/core/src/scheduler.ts @@ -23,6 +23,10 @@ export const scheduleTask = (task: Task) => { scheduledTasks.add(task) } +export const cancelScheduledTask = (task: Task) => { + scheduledTasks.delete(task) +} + /** * Starts a batch, scheduling Facet updates within the cb to be executed at the end of the batch. * @param b will be executed immediately to collect Facet changes From 7bebd8d0e7d3c2b4613d1cc30ebb13a398ca2543 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 8 Jun 2023 15:24:30 +0200 Subject: [PATCH 16/23] Testing order of execution of scheduled tasks within batches --- .../@react-facet/core/src/scheduler.spec.tsx | 48 ++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/packages/@react-facet/core/src/scheduler.spec.tsx b/packages/@react-facet/core/src/scheduler.spec.tsx index b26ec738..b0d3aaaa 100644 --- a/packages/@react-facet/core/src/scheduler.spec.tsx +++ b/packages/@react-facet/core/src/scheduler.spec.tsx @@ -2,7 +2,7 @@ import React from 'react' import { createFacet } from './facet' import { useFacetEffect, useFacetMap } from './hooks' import { mapFacetsLightweight } from './mapFacets' -import { batch } from './scheduler' +import { batch, scheduleTask } from './scheduler' import { act, render } from '@react-facet/dom-fiber-testing-library' /** @@ -54,6 +54,52 @@ it('batches maps and effects', () => { expect(effect).toHaveBeenCalledTimes(1) }) +describe('order of execution', () => { + it('runs tasks within a batch in the correct order', () => { + const order: string[] = [] + + const taskB = jest.fn().mockImplementation(() => order.push('B')) + const taskA = jest.fn().mockImplementation(() => order.push('A')) + const taskC = jest.fn().mockImplementation(() => order.push('C')) + + batch(() => { + scheduleTask(taskA) + scheduleTask(taskB) + batch(() => { + scheduleTask(taskC) + }) + }) + + expect(order).toEqual(['A', 'B', 'C']) + }) + + it('runs tasks of nested batches in the correct order', () => { + const order: string[] = [] + + const taskC = jest.fn().mockImplementation(() => { + order.push('C') + }) + const taskB = jest.fn().mockImplementation(() => { + order.push('B') + batch(() => { + scheduleTask(taskC) + }) + }) + const taskA = jest.fn().mockImplementation(() => { + order.push('A') + batch(() => { + scheduleTask(taskB) + }) + }) + + batch(() => { + scheduleTask(taskA) + }) + + expect(order).toEqual(['A', 'B', 'C']) + }) +}) + describe('mapping an array of facets', () => { it('supports batching', () => { const facetA = createFacet({ initialValue: 'a1' }) From f90b7a51c4336e91c0bd899db7e101101290981e Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 8 Jun 2023 17:56:54 +0200 Subject: [PATCH 17/23] Fix cancelation of tasks Still needs unit tests (as previous implementation proved to not be sufficient) --- packages/@react-facet/core/src/scheduler.ts | 8 ++++++-- packages/@react-facet/core/src/types.ts | 5 ++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/@react-facet/core/src/scheduler.ts b/packages/@react-facet/core/src/scheduler.ts index a13fe198..c6f507a3 100644 --- a/packages/@react-facet/core/src/scheduler.ts +++ b/packages/@react-facet/core/src/scheduler.ts @@ -24,7 +24,9 @@ export const scheduleTask = (task: Task) => { } export const cancelScheduledTask = (task: Task) => { - scheduledTasks.delete(task) + // Mark a task as canceled instead of removing it. + // Its reference might already have been taken while processing the tasks. + task.canceled = true } /** @@ -55,7 +57,9 @@ export const batch = (b: Batch) => { scheduledTasks.clear() for (const task of copiedScheduledTasks) { - task() + if (!task.canceled) { + task() + } } // Exhaust all tasks diff --git a/packages/@react-facet/core/src/types.ts b/packages/@react-facet/core/src/types.ts index 4e0b01a6..42c8da08 100644 --- a/packages/@react-facet/core/src/types.ts +++ b/packages/@react-facet/core/src/types.ts @@ -74,7 +74,10 @@ export type Option = NoValue | T /** * Unit of work that can be scheduled within a batch. */ -export type Task = () => void +export type Task = { + (): void + canceled?: boolean +} /** * Function that when executed will have its Facet updates scheduled. From 0fea214cac2cb68c6203ada1b6486c87e5408a1a Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Thu, 8 Jun 2023 19:48:44 +0200 Subject: [PATCH 18/23] Rename canceled to scheduled --- packages/@react-facet/core/src/scheduler.ts | 7 +++---- packages/@react-facet/core/src/types.ts | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/@react-facet/core/src/scheduler.ts b/packages/@react-facet/core/src/scheduler.ts index c6f507a3..5f88f417 100644 --- a/packages/@react-facet/core/src/scheduler.ts +++ b/packages/@react-facet/core/src/scheduler.ts @@ -20,13 +20,14 @@ export const scheduleTask = (task: Task) => { taskCounter.set(task, currentCount + 1) } + task.scheduled = true scheduledTasks.add(task) } export const cancelScheduledTask = (task: Task) => { // Mark a task as canceled instead of removing it. // Its reference might already have been taken while processing the tasks. - task.canceled = true + task.scheduled = false } /** @@ -57,9 +58,7 @@ export const batch = (b: Batch) => { scheduledTasks.clear() for (const task of copiedScheduledTasks) { - if (!task.canceled) { - task() - } + if (task.scheduled) task() } // Exhaust all tasks diff --git a/packages/@react-facet/core/src/types.ts b/packages/@react-facet/core/src/types.ts index 42c8da08..ac6a63d0 100644 --- a/packages/@react-facet/core/src/types.ts +++ b/packages/@react-facet/core/src/types.ts @@ -76,7 +76,7 @@ export type Option = NoValue | T */ export type Task = { (): void - canceled?: boolean + scheduled?: boolean } /** From e7df766d2f9c77091313e708fb6df7f4088c5c1b Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Sun, 11 Jun 2023 19:46:04 +0200 Subject: [PATCH 19/23] Uses an array to keep track of tasks --- packages/@react-facet/core/src/scheduler.ts | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/@react-facet/core/src/scheduler.ts b/packages/@react-facet/core/src/scheduler.ts index 5f88f417..1d4ef300 100644 --- a/packages/@react-facet/core/src/scheduler.ts +++ b/packages/@react-facet/core/src/scheduler.ts @@ -1,7 +1,7 @@ import { Task, Batch } from './types' let batchId = 0 -const scheduledTasks = new Set() +let taskQueue: Task[] = [] const taskCounter = new Map() /** @@ -20,8 +20,10 @@ export const scheduleTask = (task: Task) => { taskCounter.set(task, currentCount + 1) } - task.scheduled = true - scheduledTasks.add(task) + if (!task.scheduled) { + task.scheduled = true + taskQueue.push(task) + } } export const cancelScheduledTask = (task: Task) => { @@ -54,15 +56,18 @@ export const batch = (b: Batch) => { do { // Make a copy of the schedule, as running a task can cause other tasks to be scheduled - const copiedScheduledTasks = Array.from(scheduledTasks) - scheduledTasks.clear() + const taskQueueCopy = taskQueue + taskQueue = [] - for (const task of copiedScheduledTasks) { - if (task.scheduled) task() + for (const task of taskQueueCopy) { + if (task.scheduled) { + task.scheduled = false + task() + } } // Exhaust all tasks - } while (scheduledTasks.size > 0) + } while (taskQueue.length > 0) } // Ends a batch From f4610a323c9305e62e5df664e630c66a54471aff Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Sun, 11 Jun 2023 19:55:06 +0200 Subject: [PATCH 20/23] Using a "regular for" should be faster More info: https://esbench.com/bench/6317fc2a6c89f600a5701bc9 --- packages/@react-facet/core/src/scheduler.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/@react-facet/core/src/scheduler.ts b/packages/@react-facet/core/src/scheduler.ts index 1d4ef300..cd0bdba4 100644 --- a/packages/@react-facet/core/src/scheduler.ts +++ b/packages/@react-facet/core/src/scheduler.ts @@ -59,7 +59,9 @@ export const batch = (b: Batch) => { const taskQueueCopy = taskQueue taskQueue = [] - for (const task of taskQueueCopy) { + for (let index = 0; index < taskQueueCopy.length; index++) { + const task = taskQueueCopy[index] + if (task.scheduled) { task.scheduled = false task() From 6b49be39353ecb40ccf93dfe9e5253daa6eca66e Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Sun, 11 Jun 2023 20:06:40 +0200 Subject: [PATCH 21/23] Unit test canceling tasks --- .../@react-facet/core/src/scheduler.spec.tsx | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/@react-facet/core/src/scheduler.spec.tsx b/packages/@react-facet/core/src/scheduler.spec.tsx index b0d3aaaa..8e9966aa 100644 --- a/packages/@react-facet/core/src/scheduler.spec.tsx +++ b/packages/@react-facet/core/src/scheduler.spec.tsx @@ -2,7 +2,7 @@ import React from 'react' import { createFacet } from './facet' import { useFacetEffect, useFacetMap } from './hooks' import { mapFacetsLightweight } from './mapFacets' -import { batch, scheduleTask } from './scheduler' +import { batch, cancelScheduledTask, scheduleTask } from './scheduler' import { act, render } from '@react-facet/dom-fiber-testing-library' /** @@ -98,6 +98,23 @@ describe('order of execution', () => { expect(order).toEqual(['A', 'B', 'C']) }) + + it('cancels tasks while executing prior tasks', () => { + const order: string[] = [] + + const taskB = jest.fn().mockImplementation(() => order.push('B')) + const taskA = jest.fn().mockImplementation(() => { + order.push('A') + cancelScheduledTask(taskB) + }) + + batch(() => { + scheduleTask(taskA) + scheduleTask(taskB) + }) + + expect(order).toEqual(['A']) + }) }) describe('mapping an array of facets', () => { From b6086defd7f83fbadc9a73945804d9dabaaf48f3 Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Sun, 11 Jun 2023 20:12:36 +0200 Subject: [PATCH 22/23] Cleanup development log, plus some extra docs --- packages/@react-facet/core/src/scheduler.ts | 25 ++++++--------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/packages/@react-facet/core/src/scheduler.ts b/packages/@react-facet/core/src/scheduler.ts index cd0bdba4..e2e90a3d 100644 --- a/packages/@react-facet/core/src/scheduler.ts +++ b/packages/@react-facet/core/src/scheduler.ts @@ -2,30 +2,29 @@ import { Task, Batch } from './types' let batchId = 0 let taskQueue: Task[] = [] -const taskCounter = new Map() /** * Schedules a given task to be executed at the end of a current batch, or runs it immediately if no batch is active. * @param task - * @returns */ export const scheduleTask = (task: Task) => { + // Not currently within a batch, so we execute the task immediately. if (batchId === 0) { task() return } - if (process.env.NODE_ENV === 'development') { - const currentCount = taskCounter.get(task) ?? 0 - taskCounter.set(task, currentCount + 1) - } - + // Only schedules a task once within this batch execution. if (!task.scheduled) { task.scheduled = true taskQueue.push(task) } } +/** + * Cancels the scheduling of a previously scheduled task. + * @param task + */ export const cancelScheduledTask = (task: Task) => { // Mark a task as canceled instead of removing it. // Its reference might already have been taken while processing the tasks. @@ -44,18 +43,8 @@ export const batch = (b: Batch) => { // If this is the root batch, we start executing the tasks if (batchId === 1) { - if (process.env.NODE_ENV === 'development') { - const taskCounterCopy = Array.from(taskCounter) - taskCounter.clear() - - const optimizedCount = taskCounterCopy.filter((pair) => pair[1] > 1).length - if (taskCounterCopy.length > 0) { - console.log(`⚒️ Total: ${taskCounterCopy.length}. Optimized: ${optimizedCount}`) - } - } - do { - // Make a copy of the schedule, as running a task can cause other tasks to be scheduled + // Starts a new queue, as we work through the current one const taskQueueCopy = taskQueue taskQueue = [] From 2c3b273c969b89c2f5c228d331e28f9596e73b4f Mon Sep 17 00:00:00 2001 From: Paulo Ragonha Date: Mon, 12 Jun 2023 16:23:55 +0200 Subject: [PATCH 23/23] Avoids scheduling for first event within a batch Also properly handles exceptions within a batch. --- .../core/src/hooks/useFacetEffect.tsx | 6 +- .../core/src/mapFacets/mapIntoObserveArray.ts | 6 +- .../@react-facet/core/src/scheduler.spec.tsx | 59 ++++++++++++++++++- packages/@react-facet/core/src/scheduler.ts | 49 +++++++++------ 4 files changed, 99 insertions(+), 21 deletions(-) diff --git a/packages/@react-facet/core/src/hooks/useFacetEffect.tsx b/packages/@react-facet/core/src/hooks/useFacetEffect.tsx index 80ee3513..a52f33c4 100644 --- a/packages/@react-facet/core/src/hooks/useFacetEffect.tsx +++ b/packages/@react-facet/core/src/hooks/useFacetEffect.tsx @@ -50,7 +50,11 @@ export const createUseFacetEffect = (useHook: typeof useEffect | typeof useLayou facets.forEach((facet, index) => { unsubscribes[index] = facet.observe((value) => { values[index] = value - scheduleTask(task) + if (hasAllDependencies) { + scheduleTask(task) + } else { + task() + } }) }) diff --git a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts index 2f729b4a..a35b16f5 100644 --- a/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts +++ b/packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts @@ -65,7 +65,11 @@ export function mapIntoObserveArray( const subscriptions = facets.map((facet, index) => { return facet.observe((value) => { dependencyValues[index] = value - scheduleTask(task) + if (hasAllDependencies) { + scheduleTask(task) + } else { + task() + } }) }) diff --git a/packages/@react-facet/core/src/scheduler.spec.tsx b/packages/@react-facet/core/src/scheduler.spec.tsx index 8e9966aa..6bd1df92 100644 --- a/packages/@react-facet/core/src/scheduler.spec.tsx +++ b/packages/@react-facet/core/src/scheduler.spec.tsx @@ -37,7 +37,14 @@ it('batches maps and effects', () => { } const scenario = - render(scenario) + + // Guarantees that within a batch, we execute the effect task immediately (if the data is available on mount) + batch(() => { + render(scenario) + expect(effect).toHaveBeenCalledWith('Initial Name', 'Initial Login') + expect(effect).toHaveBeenCalledTimes(1) + }) + effect.mockClear() cleanup.mockClear() @@ -115,6 +122,37 @@ describe('order of execution', () => { expect(order).toEqual(['A']) }) + + it('handles exceptions gracefully', () => { + let order: string[] = [] + + const taskC = jest.fn().mockImplementation(() => order.push('C')) + const taskB = jest.fn().mockImplementation(() => { + throw new Error('Task failed') + }) + const taskA = jest.fn().mockImplementation(() => order.push('A')) + + // Once a batch fails + expect(() => { + batch(() => { + scheduleTask(taskA) + scheduleTask(taskB) + scheduleTask(taskC) + }) + }).toThrow() + + // Validate that we have executed the tasks until the exception + expect(order).toEqual(['A']) + + // And starting a new batch, should work just fine + order = [] + batch(() => { + scheduleTask(taskA) + scheduleTask(taskC) + }) + + expect(order).toEqual(['A', 'C']) + }) }) describe('mapping an array of facets', () => { @@ -139,6 +177,25 @@ describe('mapping an array of facets', () => { expect(observer).toHaveBeenCalledWith('a2 b2') }) + it('avoids scheduling (within a batch) if its the first event of a subscription', () => { + const facetA = createFacet({ initialValue: 'a1' }) + const facetB = createFacet({ initialValue: 'b1' }) + const facetAB = mapFacetsLightweight([facetA, facetB], (a, b) => `${a} ${b}`) + + const observer = jest.fn() + + batch(() => { + facetAB.observe(observer) + + // It should fire immediately, and not after the batch + expect(observer).toHaveBeenCalledTimes(1) + expect(observer).toHaveBeenCalledWith('a1 b1') + }) + + // Didn't call again after the batch ended + expect(observer).toHaveBeenCalledTimes(1) + }) + it('supports batching, but nested', () => { const facetA = createFacet({ initialValue: 'a1' }) const facetB = createFacet({ initialValue: 'b1' }) diff --git a/packages/@react-facet/core/src/scheduler.ts b/packages/@react-facet/core/src/scheduler.ts index e2e90a3d..96328d04 100644 --- a/packages/@react-facet/core/src/scheduler.ts +++ b/packages/@react-facet/core/src/scheduler.ts @@ -39,28 +39,41 @@ export const batch = (b: Batch) => { // Starts a batch batchId += 1 - b() + try { + b() - // If this is the root batch, we start executing the tasks - if (batchId === 1) { - do { - // Starts a new queue, as we work through the current one - const taskQueueCopy = taskQueue - taskQueue = [] + // If this is the root batch, we start executing the tasks + if (batchId === 1) { + do { + // Starts a new queue, as we work through the current one + const taskQueueCopy = taskQueue + taskQueue = [] - for (let index = 0; index < taskQueueCopy.length; index++) { - const task = taskQueueCopy[index] + try { + for (let index = 0; index < taskQueueCopy.length; index++) { + const task = taskQueueCopy[index] - if (task.scheduled) { - task.scheduled = false - task() + if (task.scheduled) { + task.scheduled = false + task() + } + } + } catch (e) { + // If something goes wrong, we unschedule all remaining tasks + for (let index = 0; index < taskQueueCopy.length; index++) { + const task = taskQueueCopy[index] + task.scheduled = false + } + + taskQueue = [] + throw e } - } - // Exhaust all tasks - } while (taskQueue.length > 0) + // Exhaust all tasks + } while (taskQueue.length > 0) + } + } finally { + // Ends a batch + batchId -= 1 } - - // Ends a batch - batchId -= 1 }