From ceda4b69b76a1a274b393cc6a1c017deca1b031c Mon Sep 17 00:00:00 2001 From: Travis Vachon Date: Mon, 4 Dec 2023 15:39:10 -0800 Subject: [PATCH] chore: extract and test the datamodel logic from `Provider` (#602) One of the core goals here was to fix a bug where the client wouldn't get reset when servicePrincipal or connection changed. The easiest way to make sure this was tested was to extract all of the datamodel creation and management logic to a dedicated hook and test that. Thanks to @gobengo for pointing out the bug and suggesting the fix: https://github.com/web3-storage/w3ui/pull/595#discussion_r1411060383 --- packages/core/src/index.ts | 1 + packages/react/package.json | 6 +- packages/react/src/hooks.ts | 77 +++++++++++++++++++++++ packages/react/src/providers/Provider.tsx | 53 ++-------------- packages/react/test/hooks.spec.ts | 52 +++++++++++++++ pnpm-lock.yaml | 42 +++++++++++++ 6 files changed, 181 insertions(+), 50 deletions(-) create mode 100644 packages/react/src/hooks.ts create mode 100644 packages/react/test/hooks.spec.ts diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index d7e304af..b45ffa54 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -14,6 +14,7 @@ export type Store = Driver const DB_NAME = '@w3ui' const DB_STORE_NAME = 'core' +export const STORE_SAVE_EVENT = 'store:save' export interface ContextState { /** diff --git a/packages/react/package.json b/packages/react/package.json index 729cadbd..7d4fc27c 100644 --- a/packages/react/package.json +++ b/packages/react/package.json @@ -36,7 +36,10 @@ "ariakit-react-utils": "0.17.0-next.27" }, "devDependencies": { + "eslint-plugin-react-hooks": "^4.6.0", + "@ipld/dag-ucan": "^3.2.0", "@testing-library/react": "^13.4.0", + "@testing-library/react-hooks": "^8.0.1", "@testing-library/user-event": "^14.4.3", "@ucanto/interface": "^9.0.0", "@ucanto/principal": "^9.0.0", @@ -47,7 +50,8 @@ }, "eslintConfig": { "extends": [ - "../../eslint.packages.js" + "../../eslint.packages.js", + "plugin:react-hooks/recommended" ] }, "eslintIgnore": [ diff --git a/packages/react/src/hooks.ts b/packages/react/src/hooks.ts new file mode 100644 index 00000000..497a5e1d --- /dev/null +++ b/packages/react/src/hooks.ts @@ -0,0 +1,77 @@ +import type { + Client, + Space, + Account, + ServiceConfig +} from '@w3ui/core' + +import { useState, useEffect, useCallback } from 'react' +import { STORE_SAVE_EVENT, createClient } from '@w3ui/core' + +export type DatamodelProps = ServiceConfig + +export interface Datamodel { + client?: Client + accounts: Account[] + spaces: Space[] + logout: () => Promise +} + +export function useDatamodel ({ servicePrincipal, connection }: DatamodelProps): Datamodel { + const [client, setClient] = useState() + const [events, setEvents] = useState() + const [accounts, setAccounts] = useState([]) + const [spaces, setSpaces] = useState([]) + + // update this function any time servicePrincipal or connection change + const setupClient = useCallback( + async (): Promise => { + const { client, events } = await createClient({ servicePrincipal, connection }) + setClient(client) + setEvents(events) + setAccounts(Object.values(client.accounts())) + setSpaces(client.spaces()) + }, + [servicePrincipal, connection] + ) + + // run setupClient once each time it changes + useEffect( + () => { + void setupClient() + }, + [setupClient] + ) + + // set up event listeners to refresh accounts and spaces when + // the store:save event from @w3ui/core happens + useEffect(() => { + if ((client === undefined) || (events === undefined)) return + + const handleStoreSave: () => void = () => { + setAccounts(Object.values(client.accounts())) + setSpaces(client.spaces()) + } + + events.addEventListener(STORE_SAVE_EVENT, handleStoreSave) + return () => { + events?.removeEventListener(STORE_SAVE_EVENT, handleStoreSave) + } + }, [client, events]) + + const logout = async (): Promise => { + // it's possible that setupClient hasn't been run yet - run createClient here + // to get a reliable handle on the latest store + const { store } = await createClient({ servicePrincipal, connection }) + await store.reset() + // set state back to defaults + setClient(undefined) + setEvents(undefined) + setAccounts([]) + setSpaces([]) + // set state up again + await setupClient() + } + + return { client, accounts, spaces, logout } +} diff --git a/packages/react/src/providers/Provider.tsx b/packages/react/src/providers/Provider.tsx index a2e210bf..2c8e9a86 100644 --- a/packages/react/src/providers/Provider.tsx +++ b/packages/react/src/providers/Provider.tsx @@ -1,14 +1,11 @@ import type { - Client, ContextState, ContextActions, - ServiceConfig, - Space, - Account + ServiceConfig } from '@w3ui/core' -import React, { createContext, useState, useContext, useEffect, ReactNode } from 'react' -import { createClient } from '@w3ui/core' +import React, { createContext, useContext, ReactNode } from 'react' +import { useDatamodel } from '../hooks' export { ContextState, ContextActions } @@ -46,49 +43,7 @@ export function Provider ({ servicePrincipal, connection }: ProviderProps): ReactNode { - const [client, setClient] = useState() - const [events, setEvents] = useState() - const [accounts, setAccounts] = useState([]) - const [spaces, setSpaces] = useState([]) - - useEffect(() => { - if ((client === undefined) || (events === undefined)) return - - const handleStoreSave: () => void = () => { - setAccounts(Object.values(client.accounts())) - setSpaces(client.spaces()) - } - - events.addEventListener('store:save', handleStoreSave) - return () => { - events?.removeEventListener('store:save', handleStoreSave) - } - }, [client, events]) - - const setupClient = async (): Promise => { - const { client, events } = await createClient({ servicePrincipal, connection }) - setClient(client) - setEvents(events) - setAccounts(Object.values(client.accounts())) - setSpaces(client.spaces()) - } - - const logout = async (): Promise => { - // it's possible that setupClient hasn't been run yet - run createClient here - // to get a reliable handle on the latest store - const { store } = await createClient({ servicePrincipal, connection }) - await store.reset() - // set state back to defaults - setClient(undefined) - setEvents(undefined) - setAccounts([]) - setSpaces([]) - // set state up again - await setupClient() - } - - useEffect(() => { void setupClient() }, []) // load client - once. - + const { client, accounts, spaces, logout } = useDatamodel({ servicePrincipal, connection }) return ( {children} diff --git a/packages/react/test/hooks.spec.ts b/packages/react/test/hooks.spec.ts new file mode 100644 index 00000000..5232a80b --- /dev/null +++ b/packages/react/test/hooks.spec.ts @@ -0,0 +1,52 @@ +import { test, expect } from 'vitest' +import 'fake-indexeddb/auto' +import { renderHook } from '@testing-library/react-hooks' +import * as DID from '@ipld/dag-ucan/did' +import { Principal, ConnectionView } from '@ucanto/interface' +import { connect } from '@ucanto/client' +import { CAR, HTTP } from '@ucanto/transport' + +import { useDatamodel } from '../src/hooks' + +test('should create a new client instance if and only if servicePrincipal or connection change', async () => { + let servicePrincipal: Principal = DID.parse('did:web:web3.storage') + let connection: ConnectionView = connect({ + id: servicePrincipal, + codec: CAR.outbound, + channel: HTTP.open({ + url: new URL('https://up.web3.storage'), + method: 'POST' + }) + }) + const { result, rerender, waitForValueToChange } = renderHook(() => useDatamodel({ servicePrincipal, connection })) + // wait for client to be initialized + await waitForValueToChange(() => result.current.client) + + const firstClient = result.current.client + expect(firstClient).not.toBeFalsy() + + rerender() + expect(result.current.client).toBe(firstClient) + + servicePrincipal = DID.parse('did:web:web3.porridge') + rerender() + // wait for the client to change + await waitForValueToChange(() => result.current.client) + // this is a little superfluous - if it's false then the line before this will hang + // I still think it's worth keeping to illustrate the point + expect(result.current.client).not.toBe(firstClient) + const secondClient = result.current.client + + connection = connect({ + id: servicePrincipal, + codec: CAR.outbound, + channel: HTTP.open({ + url: new URL('https://up.web3.porridge'), + method: 'POST' + }) + }) + rerender() + await waitForValueToChange(() => result.current.client) + expect(result.current.client).not.toBe(firstClient) + expect(result.current.client).not.toBe(secondClient) +}) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 0c317bb3..10857a22 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -610,9 +610,15 @@ importers: specifier: ^16.8.0 || ^17.0.0 || ^18.0.0 version: 18.2.0 devDependencies: + '@ipld/dag-ucan': + specifier: ^3.2.0 + version: 3.4.0 '@testing-library/react': specifier: ^13.4.0 version: 13.4.0(react-dom@18.2.0)(react@18.2.0) + '@testing-library/react-hooks': + specifier: ^8.0.1 + version: 8.0.1(@types/react@18.2.39)(react-dom@18.2.0)(react@18.2.0) '@testing-library/user-event': specifier: ^14.4.3 version: 14.5.1(@testing-library/dom@9.3.3) @@ -622,6 +628,9 @@ importers: '@ucanto/principal': specifier: ^9.0.0 version: 9.0.0 + eslint-plugin-react-hooks: + specifier: ^4.6.0 + version: 4.6.0(eslint@8.54.0) multiformats: specifier: ^11.0.1 version: 11.0.2 @@ -2909,6 +2918,29 @@ packages: pretty-format: 27.5.1 dev: true + /@testing-library/react-hooks@8.0.1(@types/react@18.2.39)(react-dom@18.2.0)(react@18.2.0): + resolution: {integrity: sha512-Aqhl2IVmLt8IovEVarNDFuJDVWVvhnr9/GCU6UUnrYXwgDFF9h2L2o2P9KBni1AST5sT6riAyoukFLyjQUgD/g==} + engines: {node: '>=12'} + peerDependencies: + '@types/react': ^16.9.0 || ^17.0.0 + react: ^16.9.0 || ^17.0.0 + react-dom: ^16.9.0 || ^17.0.0 + react-test-renderer: ^16.9.0 || ^17.0.0 + peerDependenciesMeta: + '@types/react': + optional: true + react-dom: + optional: true + react-test-renderer: + optional: true + dependencies: + '@babel/runtime': 7.23.4 + '@types/react': 18.2.39 + react: 18.2.0 + react-dom: 18.2.0(react@18.2.0) + react-error-boundary: 3.1.4(react@18.2.0) + dev: true + /@testing-library/react@13.4.0(react-dom@18.2.0)(react@18.2.0): resolution: {integrity: sha512-sXOGON+WNTh3MLE9rve97ftaZukN3oNf2KjDy7YTx6hcTO2uuLHuCGynMDhFwGw/jYf4OJ2Qk0i4i79qMNNkyw==} engines: {node: '>=12'} @@ -7444,6 +7476,16 @@ packages: react: 18.2.0 scheduler: 0.23.0 + /react-error-boundary@3.1.4(react@18.2.0): + resolution: {integrity: sha512-uM9uPzZJTF6wRQORmSrvOIgt4lJ9MC1sNgEOj2XGsDTRE4kmpWxg7ENK9EWNKJRMAOY9z0MuF4yIfl6gp4sotA==} + engines: {node: '>=10', npm: '>=6'} + peerDependencies: + react: '>=16.13.1' + dependencies: + '@babel/runtime': 7.23.4 + react: 18.2.0 + dev: true + /react-is@16.13.1: resolution: {integrity: sha512-24e6ynE2H+OKt4kqsOvNd8kBpV65zoxbA4BVsEOB3ARVWQki/DHzaUoC5KuON/BiccDaCCTZBuOcfZs70kR8bQ==} dev: true