feat: Return default value whenever query function returns undefined.#57
feat: Return default value whenever query function returns undefined.#57
Conversation
| r: Subscribable<Tx, D> | null | undefined, | ||
| query: (tx: Tx) => Promise<R>, | ||
| def: R, | ||
| export function useSubscribe<Tx, Data, Default>( |
There was a problem hiding this comment.
Just renaming template params to improve readability.
| render(<A rep={rep} />, div); | ||
| await sleep(1); | ||
| expect(renderLog).to.deep.equal(['render A', null, 'render B', null, null]); | ||
| expect(renderLog).to.deep.equal([ |
There was a problem hiding this comment.
I have looked at this pretty hard and I am sure that this expectation is actually correct.
The subscription returns the default, then the query runs and re-fires the subscription, which re-renders.
I cannot explain why my change made it faster-enough to fit under the 1ms but somehow it does.
There was a problem hiding this comment.
I don't think this is right :-)
With the new code it renders twice. That is not needed. Nothing changed.
The subscribe body returns the same value as the default value so that should not trigger a render.
OK so I still think the implementation of `useSubscribe` is fine. What was happening with the test was that `setState` *does* skip re-renders if the value is identical via Object.is(). When I changed the semantics of useSubscribe() I didn't change some of the test code to match. Previously the test code was manually coallescing undefined to null. Because the snapshot state inside useSubscribe now starts out as undefined rather than `def` this meant that when the query returned, it caused a re-render.
| ): R { | ||
| const [snapshot, setSnapshot] = useState<R>(def); | ||
| ) { | ||
| const [snapshot, setSnapshot] = useState<QueryRet | undefined>(undefined); |
There was a problem hiding this comment.
I find it easier to reason about this by setting the state to def here.
But both should behave the same so your call here.
| deps: Array<unknown> = [], | ||
| ): R { | ||
| const [snapshot, setSnapshot] = useState<R>(def); | ||
| ) { |
There was a problem hiding this comment.
Please write out the return type here. The inferred type is pretty cryptic.
export type RemoveUndefined<T> = T extends undefined ? never : T;...
): Default | RemoveUndefined<QueryRet> {
2 participants
feat: Return default value whenever query function returns undefined.