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

Batching support #122

merged 24 commits into from
Jun 15, 2023

Conversation

pirelenito
Copy link
Member

@pirelenito pirelenito commented Jun 8, 2023

The main motivation is to prevent execution of effects with "transient" values. The pseudo-code below explains in a better way a very common scenario when working with Facets:

type User = { name: string; login: string }

// Starting with a single Facet
const [user, setUser] = useFacetState<User>({ name: 'Initial Name', login: 'Initial Login' })

// We might map it into more specific properties
const nameFacet = useFacetMap(({ name }) => name, [], [user])
const loginFacet = useFacetMap(({ login }) => login, [], [user])

// But latter, we could have an effect that depends on both values
useFacetEffect(
  (name, login) => {
    // Before batching, this effect would be called twice
    // - First time: with name containing the new value, but login with old
    // - Second time : with both name and login containing their new values
  },
  [],
  [nameFacet, loginFacet],
)

Batching by default

It changes the default implementation of createFacet to automatically start a batch when its updated, so consumers will get the benefit of batching without having to update any of their code.

Batching multiple Facets

In addition, it does introduce a new API batch, that allows batching together multiple Facet updates.

batch(() => {
  facetA.set('new value')
  facetB.set('something else')
})

This API was inspired by existing community patterns, such as Solid's batch.

Performance

Benchmarks done in the internal Mojang codebase showed either an improvement or similar performance after this change (backed by the benchmarks in this repository).

Release candidates

To try this out, we've been publishing RCs at batch...batch-rc

@pirelenito pirelenito requested a review from xaviervia June 8, 2023 09:11
@@ -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.

@pirelenito pirelenito merged commit c5c7887 into main Jun 15, 2023
12 checks passed
@pirelenito pirelenito deleted the batch branch June 15, 2023 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants