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

feat(useDataEngine): make engine instance a referential constant #934

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mohammer5
Copy link
Contributor

I was trying to ensure that a hook preserves the referential identity of a function with jest by using the rerender function returned by @testing-library/react-hooks's renderHook function. This will render the wrapper twice as well, which causes the engine object to be created again. Any hook that makes use of memoisation doesn't work with this, so I created a hook called useConst (inspired by @amcgee) and made sure that the engine only gets created once.

// a hook that a jest test might want to test
const useMemoisation = () => {
  const engine = useDataEngine()
  const memoisedCallback = useCallback(() => engine.query(...), [engine])

  return memoisedCallback
}

// test
test('memoisation', () => {
  const { result, rerender } = useMemoisation()
  rerender()
  expect(result.all).toHaveLengthOf(2)
  expect(result.all[0]).toBe(result.current) 
})

@Mohammer5 Mohammer5 force-pushed the Wrap_engine_in_const_in_custom_provider branch from 73ffb74 to 631e674 Compare August 5, 2021 11:38
@Mohammer5 Mohammer5 changed the title feat(custom data provider): make engine instance a referential constant feat(use data engine hook): make engine instance a referential constant Aug 5, 2021
mediremi
mediremi previously approved these changes Aug 5, 2021
@mediremi mediremi dismissed their stale review August 5, 2021 12:25

This change assumes that apiVersion and baseUrl won't change throughout an app's lifetime (as these changing would mean we need a new link and engine)

Copy link
Member

@amcgee amcgee left a comment

Choose a reason for hiding this comment

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

this is a good change! Here's a suggestion which might address what @mediremi mentioned (though I think those cases should be rare)

Comment on lines +5 to +8
function useConst<T>(initializer: () => T): T {
const [value] = useState(initializer)
return value
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function useConst<T>(initializer: () => T): T {
const [value] = useState(initializer)
return value
}
function useInstanceVariable<T>(initializer: () => T, dependencies: any[]): T {
const [value, setValue] = useState(initializer)
const initializedRef = useRef(false)
useEffect(() => {
if (!initializedRef.current) {
initializedRef.current = true
return
}
setValue(initializer())
}, dependencies)
return value
}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like this instead, to support updates when deps change?

Copy link
Member

Choose a reason for hiding this comment

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

Note however that this will cause double-renders on dependency updates...

Copy link
Contributor

Choose a reason for hiding this comment

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

useInstanceVariable would be used in DataProvider and CustomDataProvider instead of in useDataEngine right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds a lot like we could just use useMemo instead of useConst?

Copy link
Member

Choose a reason for hiding this comment

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

@Mohammer5 this is super old... but no, there's a difference because useConst guarantees referential integrity while useMemo might re-initialize in certain situations since it is only considered a performance optimization.

Copy link
Contributor Author

@Mohammer5 Mohammer5 Dec 16, 2021

Choose a reason for hiding this comment

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

Yeah, I also figured out the other day that useMemo does not guarantee that..
I think for simplicity's sake, the following might be the most pragmatic with the least amount of abstractions:

function useDataEngine = (): DataEngine => {
    const context = useContext(DataContext)
    const [instance, setInstance] = useState(context.engine)
    
   useEffect(() => {
        setInstance(context.engine)
    }, [context.engine])

    return value
}

@amcgee amcgee changed the title feat(use data engine hook): make engine instance a referential constant feat(use-data-engine): make engine instance a referential constant Dec 16, 2021
@amcgee amcgee changed the title feat(use-data-engine): make engine instance a referential constant feat(useDataEngine): make engine instance a referential constant Dec 16, 2021
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.

3 participants