Repository navigation
feat(useList): accept readonly array inputs - #575
GamjaIsMine02 wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 33e3d78 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #575 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 69 69
Lines 2281 2281
Branches 737 737
=========================================
Hits 2281 2281 🚀 New features to boost your workflow:
|
| * const [list, actions] = useList<string>(initialItems); | ||
| * | ||
| * // Add an item | ||
| * actions.push('cherry'); |
There was a problem hiding this comment.
why did you changed example here?
I think 'as const' makes it confusing
(also I think generic type in here is unnecessary)
const [list, actions] = useList(['apple', 'banana']);I suggest this example
what do you think about this? I wanna know why did you changed here
There was a problem hiding this comment.
I changed the example to demonstrate the newly supported readonly input. With as const, the element type is narrowed to 'apple' | 'banana', so I kept <string> to allow the later calls such as push('orange').
But I agree that this unnecessarily complicates an example meant to show the hook’s basic usage! I’ll restore the original example and cover readonly inputs in the tests. Thanks for pointing this out!
| const result = renderHookSSR.serverOnly(() => useList(['a', 'b'])); | ||
| const initialState = ['a', 'b'] as const; | ||
| const result = renderHookSSR.serverOnly(() => useList(initialState)); | ||
|
|
||
| expect(result.current[0]).toEqual(['a', 'b']); | ||
| expectTypeOf(result.current[0]).toEqualTypeOf<ReadonlyArray<'a' | 'b'>>(); | ||
| }); |
There was a problem hiding this comment.
why did you change this test?
There was a problem hiding this comment.
I intended to add coverage for readonly tuple inputs, but changing the existing test was a mistake. I’ve restored the original SSR test and moved the readonly tuple inference check into a separate test case!
GamjaIsMine02
left a comment
GamjaIsMine02
left a comment
There was a problem hiding this comment.
Thanks for the review!
I simplified the usage examples as suggested and restored the original SSR test. The readonly tuple inference check is now covered in a separate test case.
| * const [list, actions] = useList<string>(initialItems); | ||
| * | ||
| * // Add an item | ||
| * actions.push('cherry'); |
There was a problem hiding this comment.
I changed the example to demonstrate the newly supported readonly input. With as const, the element type is narrowed to 'apple' | 'banana', so I kept <string> to allow the later calls such as push('orange').
But I agree that this unnecessarily complicates an example meant to show the hook’s basic usage! I’ll restore the original example and cover readonly inputs in the tests. Thanks for pointing this out!
| const result = renderHookSSR.serverOnly(() => useList(['a', 'b'])); | ||
| const initialState = ['a', 'b'] as const; | ||
| const result = renderHookSSR.serverOnly(() => useList(initialState)); | ||
|
|
||
| expect(result.current[0]).toEqual(['a', 'b']); | ||
| expectTypeOf(result.current[0]).toEqualTypeOf<ReadonlyArray<'a' | 'b'>>(); | ||
| }); |
There was a problem hiding this comment.
I intended to add coverage for readonly tuple inputs, but changing the existing test was a mistake. I’ve restored the original SSR test and moved the readonly tuple inference check into a separate test case!
|
One small request for next time @GamjaIsMine02 if you change something that this PR doesn't need (like the example or the old test here) Also, please check your diff once before you ask for a review 🙏 |
This branch has not been deployed
3 participants
Overview
useListreturns a readonly array, but its initial state andsetAllcurrently require mutable arrays. Callers must copy readonly arrays just to satisfy these parameter types.This PR accepts
readonly T[]for both inputs. A list returned byuseListcan be passed directly as anotheruseList's initial state or tosetAll, without a caller-side array copy. Mutable arrays remain valid inputs.Internal state is also typed as
readonly T[], allowing TypeScript to catch accidental in-place array mutations during future implementation changes.Tests cover readonly tuple inference, updates and reset with a frozen initial array, and passing frozen arrays and returned lists to
setAll.The JSDoc, generated English documentation, all four translations, and generated plugin reference are updated. A patch changeset is included.
Checklist
yarn run fixto format and lint the code and docs?yarn run test:coverageto make sure there is no uncovered line?