Skip to content
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

Boolean field atom does not fit useFieldAtom type. #18

Closed
MiroslavPetrik opened this issue Jan 28, 2023 · 4 comments
Closed

Boolean field atom does not fit useFieldAtom type. #18

MiroslavPetrik opened this issue Jan 28, 2023 · 4 comments

Comments

@MiroslavPetrik
Copy link
Member

Having atom with boolean value will display type error with the useFieldAtom

As a reproduction demo I have simple form with 2 checkbox inputs:
https://codesandbox.io/s/form-atoms-boolean-checkbox-field-9nv7jo?file=/src/components/checkbox-field.tsx:660-719

Expected behavior
There should be no type error.

I believe this was partly fixed for the <Field /> component in
#11.

Open question:
There is the InputField which is still constrained to the string | number | string[] and perhaps could be extended to allow boolean, as checkboxes are regular inputs of type checkbox. I believe that would be misleading, as the onChange handler for the input should be changed as well, as it uses event.target.checked instead of event.target.value https://codesandbox.io/s/form-atoms-boolean-checkbox-field-9nv7jo?file=/src/components/input-field.tsx:691-766.
So my question is whats the direction with the InputField? Would it be feasible in the future to have its props properly typed with discriminant types for various inputs e.g. of type file and others?

@jaredLunde
Copy link
Collaborator

jaredLunde commented Jan 28, 2023

Appreciate your efforts but as it relates to the abstractions this library is solving, you're philosphically incorrect. It is trivially easy for people to use <Field> or alternatively create a useCheckboxFieldAtom hook in userland. It does not need to be part of this library.
#19 (comment)

I am genuinely not seeing where the deficiency is. Here's what I do in my projects:

<Field
  atom={fieldAtoms.tos}
  render={(state, actions) => (
    <Checkbox.Label>
      <Checkbox checked={state.value} onCheckedChange={actions.setValue} />
      <span>
        I have read and agree to the{" "}
        <a href="/tos" rel="noreferrer noopener" target="_blank">
          Terms of Service
        </a>
      </span>
    </Checkbox.Label>
  )}
/>

@jaredLunde
Copy link
Collaborator

@MiroslavPetrik
Copy link
Member Author

Thank you for the quick reply, but still this is not settled for me.

I agree that the useCheckboxFieldAtom is a thing for userland, or other layer built on these excellent primitives.
From your comment I understand that the useFieldAtomProps is intended for the InputField (the props are for react component...) and so it makes sense to have the narrow type. So yes I was a bit hasty in removing it from there.

Next, given the useFieldAtom uses the useFieldAtomProps hook, it cannot have wider generic type. And this is where I think the deficiency lies. I think there should be useInputFieldAtom which would use the useFieldAtomProps, and the useFieldAtom should have unconstrained generic type.

That way you would have symmetry between hooks & react components:

  • useFieldAtom, <Field />, state, actions
  • useInputFieldAtom, <InputField />, state, actions, props

That was my mental model just from how the things are named.
Originally I stepped into this error not on the checkbox, but on upload input where I have object atom like {url: string, id: string}. Thats why I reffered to the original issue #11 .

Let me know if I should just re-learn, or if the naming will be adjusted to match the meaning.

Simply put, I imagine the fieldAtom as the most generic thing:

const aField = fieldAtom({ value: { /*anything */} });
// ...
const {state, actions} = useFieldAtom(aField);

That in turn can be used to construct the more specific hooks, like the Checkbox one.

@jaredLunde
Copy link
Collaborator

That way you would have symmetry between hooks & react components:

  • useFieldAtom, , state, actions
  • useInputFieldAtom, , state, actions, props

This makes sense to me. I think we can discuss a new major to make this breaking change happen.

@jaredLunde jaredLunde mentioned this issue Jan 28, 2023
Closed
14 tasks
jaredLunde added a commit that referenced this issue Feb 1, 2023
Refactor library to support Jotai v2 breaking changes. Rename form and field hooks to exclude
"Atom". Rename types to be consistent across the library.

BREAKING CHANGE: Renames form and field hooks to exclude "Atom" and be more terse. Renames most
exported types and several type signatures.

fix #27 #28 #18 #17
jaredLunde added a commit that referenced this issue Feb 2, 2023
- Refactor library to support Jotai v2 breaking changes. 
- Rename form and field hooks to exclude "Atom". 
- Rename types to be consistent across the library.

BREAKING CHANGE: Renames form and field hooks to exclude "Atom" and be more terse. Renames most
exported types and several type signatures.

fix #27 #28 #18 #17
github-actions bot pushed a commit that referenced this issue Feb 2, 2023
# [2.0.0-next.1](v1.3.0-next.3...v2.0.0-next.1) (2023-02-02)

### Code Refactoring

* upgrade to jotai v2 ([#29](#29)) ([e533e40](e533e40)), closes [#27](#27) [#28](#28) [#18](#18) [#17](#17)

### BREAKING CHANGES

* Renames form and field hooks to exclude "Atom" and be more terse. Renames most
exported types and several type signatures.
github-actions bot pushed a commit that referenced this issue Feb 2, 2023
# [2.0.0](v1.2.5...v2.0.0) (2023-02-02)

### Bug Fixes

* empty arrays not included in submit values ([#31](#31)) ([837140d](837140d)), closes [#26](#26)
* fix nested array walk ([#34](#34)) ([448f538](448f538))
* fix package entries ([#24](#24)) ([a18cfc5](a18cfc5))
* fix release ([#22](#22)) ([a4fff3b](a4fff3b))
* fix reset w/ initial value ([#33](#33)) ([8b49243](8b49243))

### Code Refactoring

* upgrade to jotai v2 ([#29](#29)) ([e533e40](e533e40)), closes [#27](#27) [#28](#28) [#18](#18) [#17](#17)

### Features

* add zod validator ([#23](#23)) ([06ca2c4](06ca2c4))
* bump next major ([#35](#35)) ([fb3400e](fb3400e)), closes [#20](#20)
* update build scripts and tests ([#21](#21)) ([b242e02](b242e02))

### BREAKING CHANGES

* Renames form and field hooks to exclude "Atom" and be more terse. Renames most
exported types and several type signatures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants