[CSL-3248] Refactor useCioClient to match other OS Implementations #49
Conversation
CSL-3248 OS UI PLP - Update useCioClient to be like quizzes and autocomplete for consistency
Business Outcome:
Definition of done:
|
src/hooks/useCioClient.ts
Outdated
| ) => Nullable<ConstructorIOClient> | never; | ||
| type UseCioClientProps = { | ||
| apiKey?: string; | ||
| cioClient?: any; |
There was a problem hiding this comment.
any since we might want to pass in a node client instead.
There was a problem hiding this comment.
It's very unlikely that we will pass a node client to it. Can we make it ConstructorIO from client javascript for now and change it later if we decided that we can pass a node client
There was a problem hiding this comment.
idm going down that path, but TypeScript server-side consumers might feel undervalued
src/hooks/useCioClient.ts
Outdated
| ...options, | ||
| }); | ||
| return ( | ||
| cioClient || |
There was a problem hiding this comment.
I think we can remove this line 26 ☝️
|
|
||
| it('renders CioPlp provider without children on the server without error, if client is provided', () => { | ||
| // Render the component to a string | ||
| const html = ReactDOMServer.renderToString(<CioPlp cioClient={{}} />); |
There was a problem hiding this comment.
Why we're not passing the mocked JS client here?
There was a problem hiding this comment.
I was going for an arbitrary client, but we can do that too. Scratch that, did you mean the mock client in test-utils? Wouldn't that resolve to null in this server context?
There was a problem hiding this comment.
You're correct it's not necessary
spec/CioPlp/CioPlp.test.jsx
Outdated
| }); | ||
|
|
||
| it('renders CioPlp provider without children on the client without error, if client is provided', () => { | ||
| expect(() => render(<CioPlp cioClient={{}} />)).not.toThrow(); |
There was a problem hiding this comment.
Why we're not passing the mocked JS client here?
There was a problem hiding this comment.
Same as above, I didn't see the need to specifically pass the mocked client here since an empty object is more generic but I can change that.
mocca102
left a comment
mocca102
left a comment
There was a problem hiding this comment.
I love the changes made here and thank you for adding more tests
src/hooks/useCioPlp.ts
Outdated
| export type CioPlpConfigs = { apiKey?: string }; | ||
| export type CioPlpConfigs = { | ||
| apiKey?: string; | ||
| cioClient?: any; |
There was a problem hiding this comment.
Are we changing this any to Nullable?
There was a problem hiding this comment.
Thought I changed this, thanks for catching it
Mudaafi
deleted the
csl-3248-os-ui-plp-update-usecioclient-to-be-like-quizzes-and
branch
No description provided.