-
Notifications
You must be signed in to change notification settings - Fork 3
feat: Allow providing neither a controlled nor an initial value #10
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
Changes from 3 commits
78bf7c2
293aacf
8683736
936c822
92e4280
fd38ff1
e7a0417
ddc143f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,20 @@ it('supports an uncontrolled mode', () => { | |
screen.getByText('Children'); | ||
}); | ||
|
||
it('supports an uncontrolled mode with no initial value', () => { | ||
const onChange = jest.fn(); | ||
|
||
render(<Expander onChange={onChange} />); | ||
expect(screen.queryByText('Children')).toBe(null); | ||
fireEvent.click(screen.getByText('Toggle')); | ||
expect(onChange).toHaveBeenLastCalledWith(true); | ||
|
||
screen.getByText('Children'); | ||
fireEvent.click(screen.getByText('Toggle')); | ||
expect(onChange).toHaveBeenLastCalledWith(false); | ||
expect(screen.queryByText('Children')).toBe(null); | ||
}); | ||
|
||
it('allows to use an initial value without a change handler', () => { | ||
// Maybe the value is read from the DOM directly | ||
render(<Expander initialExpanded />); | ||
|
@@ -81,12 +95,6 @@ it('uses the controlled value when both a controlled as well as an initial value | |
screen.getByText('Children'); | ||
}); | ||
|
||
it('throws when neither a controlled nor an initial value is provided', () => { | ||
expect(() => render(<Expander />)).toThrow( | ||
'Either an initial or a controlled value should be provided.' | ||
); | ||
}); | ||
|
||
it('throws when switching from uncontrolled to controlled mode', () => { | ||
const {rerender} = render(<Expander initialExpanded />); | ||
|
||
|
@@ -102,3 +110,64 @@ it('throws when switching from controlled to uncontrolled mode', () => { | |
/Can not change from controlled to uncontrolled mode./ | ||
); | ||
}); | ||
|
||
/** | ||
* Type signature tests | ||
*/ | ||
|
||
function TestTypes() { | ||
const controlled = useOptionallyControlledState({ | ||
controlledValue: true | ||
}); | ||
const uncontrolledWithInitialValue = useOptionallyControlledState({ | ||
initialValue: true | ||
}); | ||
const uncontrolledWithoutInitialValue = useOptionallyControlledState({}); | ||
|
||
// Only used for type tests; mark the variables as used | ||
// eslint-disable-next-line no-unused-expressions | ||
[controlled, uncontrolledWithInitialValue, uncontrolledWithoutInitialValue]; | ||
} | ||
|
||
// Expected return type: `[boolean, (value: boolean) => void]` | ||
function Controlled(opts: {controlledValue: boolean; initialValue?: boolean}) { | ||
const [value, setValue] = useOptionallyControlledState(opts); | ||
|
||
setValue(true); | ||
return value.valueOf(); | ||
} | ||
|
||
// Expected return type: `[boolean, (value: boolean) => void]` | ||
function UncontrolledWithInitialValue(opts: { | ||
controlledValue?: boolean; | ||
initialValue: boolean; | ||
}) { | ||
const [value, setValue] = useOptionallyControlledState(opts); | ||
|
||
setValue(true); | ||
return value.valueOf(); | ||
} | ||
|
||
// Expected return type: `[boolean | undefined, (value: boolean) => void]` | ||
function UncontrolledWithoutInitialValue(opts: { | ||
controlledValue?: boolean; | ||
initialValue?: boolean; | ||
}) { | ||
// False positive, probably need to upgrade ESLint | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
const [value, setValue] = useOptionallyControlledState(opts); | ||
|
||
setValue(true); | ||
|
||
// @ts-expect-error This should be an error as the value can be undefined | ||
return value.valueOf(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be an error but isn't unfortunately. Something seems off with the undefined checks (my playground for testing). I'm wondering when in doubt if we should rather go for a potential There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @menelaos Do you have an idea by chance? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just tried for quite a bit, but couldn't find a solution 😞 I would also suggest to proceed with the potential |
||
} | ||
|
||
// Only used for type tests; mark the functions as used | ||
// eslint-disable-next-line no-unused-expressions | ||
[ | ||
TestTypes, | ||
Controlled, | ||
UncontrolledWithInitialValue, | ||
UncontrolledWithoutInitialValue | ||
]; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2122,6 +2122,11 @@ | |
dependencies: | ||
"@babel/types" "^7.3.0" | ||
|
||
"@types/eslint-visitor-keys@^1.0.0": | ||
version "1.0.0" | ||
resolved "https://registry.yarnpkg.com/@types/eslint-visitor-keys/-/eslint-visitor-keys-1.0.0.tgz#1ee30d79544ca84d68d4b3cdb0af4f205663dd2d" | ||
integrity sha512-OCutwjDZ4aFS6PB1UZ988C4YgwlBHJd6wCeQqaLdmadZ/7e+w79+hbMUFC1QXDNCmdyoRfAFdm0RypzwR+Qpag== | ||
|
||
"@types/estree@*": | ||
version "0.0.45" | ||
resolved "https://registry.yarnpkg.com/@types/estree/-/estree-0.0.45.tgz#e9387572998e5ecdac221950dab3e8c3b16af884" | ||
|
@@ -2300,7 +2305,17 @@ | |
eslint-scope "^5.0.0" | ||
eslint-utils "^2.0.0" | ||
|
||
"@typescript-eslint/[email protected]", "@typescript-eslint/parser@^2.12.0", "@typescript-eslint/parser@^5.0.0": | ||
"@typescript-eslint/parser@^2.12.0": | ||
version "2.34.0" | ||
resolved "https://registry.yarnpkg.com/@typescript-eslint/parser/-/parser-2.34.0.tgz#50252630ca319685420e9a39ca05fe185a256bc8" | ||
integrity sha512-03ilO0ucSD0EPTw2X4PntSIRFtDPWjrVq7C3/Z3VQHRC7+13YB55rcJI3Jt+YgeHbjUdJPcPa7b23rXCBokuyA== | ||
dependencies: | ||
"@types/eslint-visitor-keys" "^1.0.0" | ||
"@typescript-eslint/experimental-utils" "2.34.0" | ||
"@typescript-eslint/typescript-estree" "2.34.0" | ||
eslint-visitor-keys "^1.1.0" | ||
|
||
"@typescript-eslint/parser@^5.0.0": | ||
version "5.20.0" | ||
resolved "https://registry.yarnpkg.com/@typescript-eslint/parser/-/parser-5.20.0.tgz#4991c4ee0344315c2afc2a62f156565f689c8d0b" | ||
integrity sha512-UWKibrCZQCYvobmu3/N8TWbEeo/EPQbS41Ux1F9XqPzGuV7pfg6n50ZrFo6hryynD8qOTTfLHtHjjdQtxJ0h/w== | ||
|
@@ -9444,11 +9459,16 @@ typescript@^3.7.3: | |
resolved "https://registry.yarnpkg.com/typescript/-/typescript-3.9.10.tgz#70f3910ac7a51ed6bef79da7800690b19bf778b8" | ||
integrity sha512-w6fIxVE/H1PkLKcCPsFqKE7Kv7QUwhU8qQY2MueZXWx5cPZdwFupLgKK3vntcK98BtNHZtAF4LA/yl2a7k8R6Q== | ||
|
||
typescript@^4.0.0, typescript@^4.6.3: | ||
typescript@^4.0.0: | ||
version "4.6.3" | ||
resolved "https://registry.yarnpkg.com/typescript/-/typescript-4.6.3.tgz#eefeafa6afdd31d725584c67a0eaba80f6fc6c6c" | ||
integrity sha512-yNIatDa5iaofVozS/uQJEl3JRWLKKGJKh6Yaiv0GLGSuhpFJe7P3SbHZ8/yjAHRQwKRoA6YZqlfjXWmVzoVSMw== | ||
|
||
typescript@^4.8.2: | ||
version "4.8.2" | ||
resolved "https://registry.yarnpkg.com/typescript/-/typescript-4.8.2.tgz#e3b33d5ccfb5914e4eeab6699cf208adee3fd790" | ||
integrity sha512-C0I1UsrrDHo2fYI5oaCGbSejwX4ch+9Y5jTQELvovfmFkK3HHSZJB8MSJcWLmCUBzQBchCrZ9rMRV6GuNrvGtw== | ||
|
||
uglify-js@^3.1.4: | ||
version "3.10.4" | ||
resolved "https://registry.yarnpkg.com/uglify-js/-/uglify-js-3.10.4.tgz#dd680f5687bc0d7a93b14a3482d16db6eba2bfbb" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO:
useOptionalState
as it delivers the same meaning ("controlled" means there's no intermediate state)