Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Batching support #122

Merged
merged 24 commits into from
Jun 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
fedbfc9
Very naive batching implementation
pirelenito Jun 2, 2023
e86aaf0
Batches effects of other batches
pirelenito Jun 2, 2023
b40aac3
Extract batch to a separated module and make it apply to Facet by def…
pirelenito Jun 2, 2023
cfc9947
WIP: logging extra usage
pirelenito Jun 7, 2023
876e796
Remove log and schedule on effects
pirelenito Jun 7, 2023
0290201
Logs scheduler success ration
pirelenito Jun 7, 2023
0b02ec7
Rename batch into scheduler
pirelenito Jun 8, 2023
131e09f
Documentation
pirelenito Jun 8, 2023
72133f6
Only starts a batch on createFacet if we are notifying a change
pirelenito Jun 8, 2023
642c8f2
Also test useFacetEffect
pirelenito Jun 8, 2023
706e845
Keep batch started until the end
pirelenito Jun 8, 2023
2233682
Fix linter
pirelenito Jun 8, 2023
f27c521
Expose batch as a public API
pirelenito Jun 8, 2023
fb55b16
Small refactor
pirelenito Jun 8, 2023
509fb14
Make sure to cancel tasks that are no longer needed
pirelenito Jun 8, 2023
7bebd8d
Testing order of execution of scheduled tasks within batches
pirelenito Jun 8, 2023
f90b7a5
Fix cancelation of tasks
pirelenito Jun 8, 2023
0fea214
Rename canceled to scheduled
pirelenito Jun 8, 2023
e7df766
Uses an array to keep track of tasks
pirelenito Jun 11, 2023
f4610a3
Using a "regular for" should be faster
pirelenito Jun 11, 2023
6b49be3
Unit test canceling tasks
pirelenito Jun 11, 2023
b6086de
Cleanup development log, plus some extra docs
pirelenito Jun 11, 2023
2c3b273
Avoids scheduling for first event within a batch
pirelenito Jun 12, 2023
c79e33d
Merge branch 'main' into batch
pirelenito Jun 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions packages/@react-facet/core/src/facet/createFacet.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { defaultEqualityCheck } from '../equalityChecks'
import { Cleanup, EqualityCheck, Listener, WritableFacet, StartSubscription, Option, NO_VALUE } from '../types'
import { batch } from '../scheduler'

export interface FacetOptions<V> {
initialValue: Option<V>
Expand Down Expand Up @@ -42,11 +43,13 @@ export function createFacet<V>({
}
}

currentValue = newValue
batch(() => {
currentValue = newValue

for (const listener of listeners) {
listener(currentValue)
}
for (const listener of listeners) {
listener(currentValue)
}
})
}

/**
Expand Down
24 changes: 17 additions & 7 deletions packages/@react-facet/core/src/hooks/useFacetEffect.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { useCallback, useEffect, useLayoutEffect } from 'react'
import { Facet, Unsubscribe, Cleanup, NO_VALUE, ExtractFacetValues } from '../types'
import { cancelScheduledTask, scheduleTask } from '../scheduler'

export const createUseFacetEffect = (useHook: typeof useEffect | typeof useLayoutEffect) => {
return function <Y extends Facet<unknown>[], T extends [...Y]>(
Expand Down Expand Up @@ -34,22 +35,31 @@ 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<T>))
}
}

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<T>))
scheduleTask(task)
} else {
task()
}
})
})

return () => {
cancelScheduledTask(task)
unsubscribes.forEach((unsubscribe) => unsubscribe())
if (cleanup != null) {
cleanup()
Expand Down
1 change: 1 addition & 0 deletions packages/@react-facet/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ export * from './helpers'
export * from './hooks'
export * from './mapFacets'
export * from './types'
export { batch } from './scheduler'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remain consistent and export with *. I know there are other two functions being exported and maybe they were meant as internal, but I think there's value in exposing them, we don't yet know if there is no use for them in third party library code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, the reason was to keep them private.

The main risk with open-source packages is that once an API is public and used... its a wider surface we need to keep compatible.

55 changes: 24 additions & 31 deletions packages/@react-facet/core/src/mapFacets/mapIntoObserveArray.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { cancelScheduledTask, scheduleTask } from '../scheduler'
import { defaultEqualityCheck } from '../equalityChecks'
import { EqualityCheck, Listener, Option, NO_VALUE, Observe, Facet, NoValue } from '../types'

Expand All @@ -14,31 +15,22 @@ export function mapIntoObserveArray<M>(
const dependencyValues: Option<unknown>[] = facets.map(() => NO_VALUE)
let hasAllDependencies = false

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)
const task =
checker == null
? () => {
hasAllDependencies = hasAllDependencies || dependencyValues.every((value) => value != NO_VALUE)
if (!hasAllDependencies) return

if (hasAllDependencies) {
const result = fn(...dependencyValues)
if (result === NO_VALUE) return

listener(result)
}
})
}

// 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)
: equalityCheck === defaultEqualityCheck
? () => {
hasAllDependencies = hasAllDependencies || dependencyValues.every((value) => value != NO_VALUE)
if (!hasAllDependencies) return

if (hasAllDependencies) {
const result = fn(...dependencyValues)
if (result === NO_VALUE) return

Expand All @@ -59,29 +51,30 @@ export function mapIntoObserveArray<M>(

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

// Finally we use the custom equality check
listener(result)
}

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)
scheduleTask(task)
} else {
task()
}
})
})

return () => {
cancelScheduledTask(task)
subscriptions.forEach((unsubscribe) => unsubscribe())
}
}
Expand Down
Loading
Loading