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

Fix useFacetCallback having outdated values #114

Merged
merged 6 commits into from
Mar 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
45 changes: 41 additions & 4 deletions packages/@react-facet/core/src/hooks/useFacetCallback.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -181,22 +181,26 @@ it('returns NO_VALUE if any facet has NO_VALUE and skip calling the callback', (
it('has proper return type with NO_VALUE in it', () => {
const facetA = createFacet({ initialValue: 'a' })

let handler: (event: string) => void

const TestComponent = () => {
const handler = useFacetCallback(
handler = useFacetCallback(
(a) => (b: string) => {
return a + b
},
[],
[facetA],
)

if (handler('string') !== NO_VALUE) {
throw new Error('Expected NO_VALUE')
}
return null
}

render(<TestComponent />)

act(() => {
const result = handler('string')
expect(result).toBe('astring')
})
})

it('returns the defaultValue, when provided, if any facet has NO_VALUE and skip calling the callback', () => {
Expand Down Expand Up @@ -229,3 +233,36 @@ it('returns the defaultValue, when provided, if any facet has NO_VALUE and skip

expect(callback).not.toHaveBeenCalledWith()
})

it('should always have the current value of tracked facets', () => {
const facetA = createFacet<string>({ initialValue: NO_VALUE })

let handler: (event: string) => void = () => {}

const TestComponent = () => {
handler = useFacetCallback(
(a) => (b: string) => {
return a + b
},
[],
[facetA],
)

return null
}

// We make sure to be the first listener registered, so this is called before
// the listener within the useFacetCallback (which would have created the issue)
facetA.observe(() => {
const result = handler('string')
expect(result).toBe('newstring')
})

render(<TestComponent />)

// In this act, the effect within useFacetCallback will be executed, subscribing for changes of the facetA
// Then we set the value, causing the listener above to being called
act(() => {
facetA.set('new')
})
})
23 changes: 11 additions & 12 deletions packages/@react-facet/core/src/hooks/useFacetCallback.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { useCallback, useLayoutEffect, useRef } from 'react'
import { Facet, NO_VALUE, Option, ExtractFacetValues, NoValue } from '../types'
import { useCallback, useLayoutEffect } from 'react'
import { Facet, NO_VALUE, ExtractFacetValues, NoValue } from '../types'

/**
* Creates a callback that depends on the value of a facet.
Expand Down Expand Up @@ -43,14 +43,10 @@ export function useFacetCallback<M, Y extends Facet<unknown>[], T extends [...Y]
facets: T,
defaultReturnValue?: M,
): (...args: K) => M | NoValue {
const facetsRef = useRef<Option<unknown>[]>(facets.map(() => NO_VALUE))

useLayoutEffect(() => {
const unsubscribes = facets.map((facet, index) => {
return facet.observe((value) => {
facetsRef.current[index] = value
})
})
// Make sure to start subscriptions, even though we are getting the values directly from them
// We read the values using `.get` to make sure they are always up-to-date
const unsubscribes = facets.map((facet) => facet.observe(noop))

return () => {
unsubscribes.forEach((unsubscribe) => unsubscribe())
Expand All @@ -63,17 +59,20 @@ export function useFacetCallback<M, Y extends Facet<unknown>[], T extends [...Y]
// eslint-disable-next-line react-hooks/exhaustive-deps
const callbackMemoized = useCallback(callback, dependencies)

// eslint-disable-next-line react-hooks/exhaustive-deps
return useCallback(
(...args: K) => {
const values = facetsRef.current
const values = facets.map((facet) => facet.get())
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be args.map()?

I'm not sure how to fix errors the the linter is throwing here.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should do it in the same way as in the useLayoutEffect above. Just pushed a new commit.


for (const value of values) {
if (value === NO_VALUE) return defaultReturnValue != null ? defaultReturnValue : NO_VALUE
}

return callbackMemoized(...(values as ExtractFacetValues<T>))(...(args as K))
},
[callbackMemoized, facetsRef, defaultReturnValue],
// We care about each individual facet and if any is a different reference
// eslint-disable-next-line react-hooks/exhaustive-deps
[callbackMemoized, defaultReturnValue, ...facets],
)
}

const noop = () => {}