Coder Social home page Coder Social logo

Comments (23)

jaredLunde avatar jaredLunde commented on September 23, 2024 2

unfortunately this will be a breaking change but I do think it has to happen... was hoping i could get away with something backward compatible but at minimum the types break

from form-atoms.

jaredLunde avatar jaredLunde commented on September 23, 2024 1

This feels very comfy to me:

Screen.Recording.2023-02-08.at.12.48.18.PM.mov

from form-atoms.

jaredLunde avatar jaredLunde commented on September 23, 2024 1

It defaults to string which works for both <textarea> and <select>. Still trying to figure out the best way to handle the <select multiple> case or wondering if it's even worth it. Things look pretty clean with the <Type, Value> generics as is:

const numberTypes = new Set(["number", "range"] as const);
const dateTypes = new Set([
  "date",
  "datetime-local",
  "month",
  "time",
  "week",
] as const);
const fileTypes = new Set(["file"] as const);

export type UseInputFieldProps<
  Type extends React.HTMLInputTypeAttribute,
  Value extends InputFieldValueForType<Type> = InputFieldValueForType<Type>
> = {
  /**
   * The name of the field if there is one
   */
  name: string | undefined;
  /**
   * The value of the field
   */
  value: Value;
  /**
   * The type of the field
   *
   * @default "text"
   */
  type: Type;
  /**
   * A WAI-ARIA property that tells a screen reader whether the
   * field is invalid
   */
  "aria-invalid": boolean;
  /**
   * A React callback ref that is used to bind the field atom to
   * an `<input>`, `<select>`, or `<textarea>` element so that it
   * can be read and focused.
   */
  ref: React.RefCallback<
    HTMLInputElement | HTMLTextAreaElement | HTMLSelectElement
  >;
  onBlur(event: React.FormEvent<HTMLInputElement>): void;
  onBlur(event: React.FormEvent<HTMLTextAreaElement>): void;
  onBlur(event: React.FormEvent<HTMLSelectElement>): void;
  onChange(event: React.ChangeEvent<HTMLInputElement>): void;
  onChange(event: React.ChangeEvent<HTMLTextAreaElement>): void;
  onChange(event: React.ChangeEvent<HTMLSelectElement>): void;
};

export type UseInputFieldPropsOptions<
  Type extends React.HTMLInputTypeAttribute
> = UseAtomOptions & {
  /**
   * The type of the `<input>` element
   *
   * @default "text"
   * @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#%3Cinput%3E_types
   */
  type?: Type;
};

export type DateType = typeof dateTypes extends Set<infer T> ? T : never;
export type NumberType = typeof numberTypes extends Set<infer T> ? T : never;
export type FileType = typeof fileTypes extends Set<infer T> ? T : never;

/**
 * A utility type that maps input types to their corresponding
 * value types.
 */
export type InputFieldValueForType<Type extends React.HTMLInputTypeAttribute> =
  Type extends NumberType
    ? number
    : Type extends DateType
    ? Date | null
    : Type extends FileType
    ? FileList | null
    : string;

from form-atoms.

MiroslavPetrik avatar MiroslavPetrik commented on September 23, 2024 1

I would ship specific hook for the select component
useSelectFieldProps
and drop the overload of event handlers e.g.
onBlur(event: React.FormEvent<HTMLSelectElement>): void;

That way we can have select options, where the multiple option can be used to switch between string or string[]

I think that would be correct, and users would have clear API

const props = useInputFieldProps(...)
<input {...props} />
const props = useSelectFieldProps(...)
<select {...props} />

Not sure about the textarea for correctness sake it should also have it's own hook as its a separate element,
when the props will include the type, maybe react will complain that we are passing unrecognized prop when it will be spread like so:

const props = useInputFieldProps(...)
<textarea {...props} /> // `type` props is not recognized

from form-atoms.

MiroslavPetrik avatar MiroslavPetrik commented on September 23, 2024 1

the issue is you are comparing types of HTMLInputtype with FileList which is always false.
image

You had intent to map it to undefined, while its a string now.

Yeah, its understandable that it does not make sense for React. I understand that you want to have the props be spreadable into the <input />, while I thought about customized UI components, but for that I can read the value separately.

from form-atoms.

jaredLunde avatar jaredLunde commented on September 23, 2024 1

Also I see that you've constrained select to have generic Value extends string. I wonder what value you want to use for empty, pristine input, is it ""? This goes back to the validation, as that is harder to validate.

As a matter of correctness, "" should always the empty value. Same goes for text/number/date inputs. That's always been the case.

  • I'm not inclined to make textarea/text inputs nullable. That does not make sense to me for the reason stated above.
  • Multi-select requires an empty [] value as that is what React expects. Correctness.
  • For numbers, if valueAsNumber is NaN then it should be coerced to null.

Edit: actually NaN might actually be correct in the case of a number because NaN is a number type whereas null is not. What you're describing is a case for the transform function I mentioned.

from form-atoms.

jaredLunde avatar jaredLunde commented on September 23, 2024 1

Yeah that's an interesting take, but circling back I think I'd rather standardize around null and keep the higher level abstractions in user land. So if valueAsNumber is NaN, make it null instead.

from form-atoms.

jaredLunde avatar jaredLunde commented on September 23, 2024

Would have to think on the best abstraction. Isn't type=number the only input that uses target.valueAsNumber? Likewise <select multiple> is the only string[]? So couldn't we just add a type and multiple prop to options and narrow based on that while forwarding the type to the output of props? Could potentially cover dates with valueAsDate here too.

I'm reluctant to do anything with checkboxes as there are plenty of cases where someone wants the value rather than the checked state. A boolean also doesn't account for indeterminate states.

from form-atoms.

jaredLunde avatar jaredLunde commented on September 23, 2024

I can't say I'm a fan of a getEventValue option. I'd be more likely to add a transform option to the atom itself as that covers more cases.

from form-atoms.

MiroslavPetrik avatar MiroslavPetrik commented on September 23, 2024

For the number datatype there is also <input type="range">. Extending option with type could work, based on that there can be switch in the onChange handler. And to get the event types right, the function types could be overloaded.

For the string[] there is also checkbox group, but that is outside of standard input elements, so that would be handled with pure useField hook. This bug should resolve problems in the useInputField and <InputField /> which deal with the native HTML elements.

I'm not sure about the transform in the atom itself, it seems to me that would couple the atoms to HTML events while currently they are not (the ref is an exception there, maybe useless in some cases). I have components which use plain useField stuff, without handling any ChangeEvents so there is nothing to transform, as I simply call actions.setValue().

Also I don't like the idea of configuring stuff by the type prop, e.g. I'm trying to model the fields in my library, by data types, so when I have NumberField or useNumberField, it can be used for both input types of range or number. The component/hook tells me what FieldAtom<type> I can supply and then it should work.

So that way the users can be sure when the field was accepted as a prop to a <Field /> component, then it will work. No need to specify another type=""

For the input hooks, I think that currying could be used:

const useInput = curry((transform, field) => props)

export const useNumericInput = useInput(event => event.target.valueAsNumber)
export const useFileInput = useInput(event =>event.target.files)
export const useDateInput = useInput(event => event.target.valueAsDate)
// ...

But yeah, it goes hand in hand with the HTMLInput type prop, so it should be one of the returned props anyway:

const props = useInput(field, {type: "date"});
// props.type === "date"


<input {...props} />
const props = useDateInput(field);
// props.type === "date"

<input {...props} />

from form-atoms.

jaredLunde avatar jaredLunde commented on September 23, 2024

I'm not sure about the transform in the atom itself, it seems to me that would couple the atoms to HTML events

It's not coupled to the event types it's coupled to the value. That's why it covers more cases. It's for when you want your value in a particular format without needing to do anything in onSubmit. For UI-only transforms, one can derive the state of the atom.

const field = fieldAtom({
  value: '',
  transform(value: string): string {
    return value.replace(/\s+/, ' ');
  }
})

The reason to discriminate by type is because it sticks to the "this is just HTML" principle. There's nothing new to learn and it's not a new API.

from form-atoms.

MiroslavPetrik avatar MiroslavPetrik commented on September 23, 2024

Look good! That will be good enough even for raw html inputs with some styles!

How do you handle select and textarea? Because those are not the HTMLInputElement.

from form-atoms.

jaredLunde avatar jaredLunde commented on September 23, 2024

yeah good points

from form-atoms.

MiroslavPetrik avatar MiroslavPetrik commented on September 23, 2024

No problem, fuck around and break things, that's how it works.

You really went into it, didn't expect so much effort!
I like your textarea implementation, had the exact same idea.

I'm testing it, and currently the types are too strict for me - I'm using the value: undefined as initial fieldAtom state in multiple components, as that plays nicely with zod, as you can use the required_message conveniently.

So the tests should have a case for pristine inputs https://github.com/jaredLunde/form-atoms/pull/38/files#r1101341484.
I'm not sure if forcing undefined onto users is good idea.
For my use case it would be sufficient if the Value type was optional Value | undefined:

image

Note that Date or FileList are nullable with | null so I've also normalized it to undefined on my side, so I have uniform usage patterns and submit values to process.

from form-atoms.

MiroslavPetrik avatar MiroslavPetrik commented on September 23, 2024

There is one minor bug with the value prop in input[type=file]

https://github.com/jaredLunde/form-atoms/pull/38/files#r1101347493

from form-atoms.

jaredLunde avatar jaredLunde commented on September 23, 2024

Why would it be FileType? Someone could in theory use an input with a multiple prop. Using FileList covers all cases.

I think you should be able to do fieldAtom<number | null>({ value: null }) but I'll have to double check. null returns an empty string for its value.

from form-atoms.

MiroslavPetrik avatar MiroslavPetrik commented on September 23, 2024

from form-atoms.

jaredLunde avatar jaredLunde commented on September 23, 2024
  • Sorry, not seeing the issue in your comment on #38. type=file components are always uncontrolled in React. FileList is not a valid value property. For this same reason, date inputs have a string type.
  • Has to be null. Has to use the same behavior Date/FileList do.

from form-atoms.

MiroslavPetrik avatar MiroslavPetrik commented on September 23, 2024

With these changes the validation pattern could be:

const optionalNumber = zodValidate(z.nullable(z.number()));

const requiredNumber = zodValidate(z.number({ invalid_type_error: "This field is required" }));

from form-atoms.

MiroslavPetrik avatar MiroslavPetrik commented on September 23, 2024

We still have to normalize empty values when the input is cleared.
For example when number field is cleared by user, the valueAsNumber returns NaN which is essentially parseFloat("") because empty input usually reads as empty string.

image

So when setting the atom value we need to turn the empty strings to null with || null.

Also I see that you've constrained select to have generic Value extends string. I wonder what value you want to use for empty, pristine input, is it ""? This goes back to the validation, as that is harder to validate.

I would suggest to have all the values nullable, even textarea, so my ideal state would be.

  1. in atoms, have empty values as null
  2. the value prop from useProps hooks will always be spreadable to elements
  3. all inputs can be validated with z.nullable(z.type()) for optional inputs and z.type({invalid_type_error: "required"}) for required inputs

Exception could be multi select, where it's ok to have [] as empty value.

from form-atoms.

MiroslavPetrik avatar MiroslavPetrik commented on September 23, 2024

Yeah, thanks for the insight.

Do we need then to make the number nullable? I've tested the NaN behavior and it can be used for empty inputs.
Also zod treats null and NaN the same way:

z.number().safeParse(NaN)); // {"code": "invalid_type", ...}
z.number().safeParse(null)); // {"code": "invalid_type", ...}

Going back to initial state where we had empty strings in submit values, now we will have NaN which is better type-wise, but still for my practical applications, I think I will nullify the NaNs. I can imagine users being surprised getting NaN in submit values.
To me it seems like a case of practicality beats purity.

It would be interesting to see your transform idea, if the value will be in submit data, or in validator or where.

from form-atoms.

jaredLunde avatar jaredLunde commented on September 23, 2024

I merged a PR that lets numbers be nullable yesterday 😀 Just want to make sure it still makes sense in the post-transform world. Will try to get to that tonight otherwise some time this weekend. Thanks again!

from form-atoms.

MiroslavPetrik avatar MiroslavPetrik commented on September 23, 2024

Redwood emptyAs prop could be an inspiration.

  1. for number type, they have NaN as empty, so possibly you can revert the nullable number commit.
  2. they have setValueAs prop, which sounds like your transform

from form-atoms.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.