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

An empty Field.String produces undefined by default, not an empty string #4070

Closed
kimroen opened this issue Oct 6, 2024 · 12 comments · Fixed by #4111 or #4174
Closed

An empty Field.String produces undefined by default, not an empty string #4070

kimroen opened this issue Oct 6, 2024 · 12 comments · Fixed by #4111 or #4174
Labels

Comments

@kimroen
Copy link
Contributor

kimroen commented Oct 6, 2024

🐛 Bug Report

This could be expected and intended behavior, and if so you can close this and consider it feedback 😊

When you empty out a Field.String, the resulting value in the form is undefined, and not '' (an empty string). This can be confusing, especially if combined with a default data value of '' when setting up the form.

To Reproduce

In this CodeSandbox, we find a form with one field which has the default value '', and a corresponding Field.String to edit it:

function MyForm() {
  return (
    <Form.Handler defaultData={{ myPath: '' }}>
      <Field.String path="/myPath" />
    </Form.Handler>
  );
}

The value of /myPath when this form is rendered is '', as expected. However, when you type a character and then remove it, the value of /myPath is undefined.

Expected behavior

I expected the value of /myPath to be an empty string after editing, not undefined.

Why is this unexpected?

If you've worked with React for any amount of time, you've probably done something like this to make a controlled input:

const [fieldValue, setFieldValue] = useState();

return (
  <input value={fieldValue} onChange={(e) => { setFieldValue(e.currentTarget.value) }} />
);

And if you have, then you've also seen this warning that shows up when you start typing:

Warning: A component is changing an uncontrolled input to be controlled. This is likely caused by the value changing from undefined to a defined value, which should not happen. Decide between using a controlled or uncontrolled input element for the lifetime of the component.

So at least for me, I instinctively give the value a default value of empty string.

As a bonus, keeping it a string makes dealing with the types easier 😊

Eufemia Version

Browser JS: 10.51.2

Browser CSS: 10.51.2

@kimroen
Copy link
Contributor Author

kimroen commented Oct 6, 2024

Now I realize that there's a property called emptyValue that we could specify to be an empty string, but two things:

  1. I expected that to be the default
  2. If you specify that and also specify an empty string in defaultData on the form, then the actual value until the field is edited is undefined. This is probably a separate bug, though

@andlbrei
Copy link
Contributor

andlbrei commented Oct 7, 2024

I'll chime in with my viewpoint based on using JSON schema for validation.

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "type": "object",
  "required": [
    "myString"
  ],
  "properties": {
    "myString": {
      "type": "string"
    }
  }
}

This will allow myString: "", but not myString: null (or the absence of the attribute).
In our case this works fine. No value in the field means it's not a string. If we would want an empty string, we'd set it with emptyValue (I don't see any case where we need to have an empty string though).
If the default would be an empty string, we would need to have a minimum length check as well to ensure the string actually has content.
Not a big deal, but more work.

@tujoworker
Copy link
Member

tujoworker commented Oct 8, 2024

I think that its unlikely that we can change that in the future. For sure can we try to create tools to make fields/components as easy as possible to use.
But when we see at other fields, like the Number field, then I think its more clear that undefined as a "data" value, is a good choice. Because we often can't have a default value.

I think its great to have a discussion around this, and any suggestion to improve the tooling are welcome 🫶

@kimroen
Copy link
Contributor Author

kimroen commented Oct 9, 2024

I think that its unlikely that we can change that in the future. For sure can we try to create tools to make fields/components as easy as possible to use.
But when we see at other fields, like the Number field, then I think its more clear that undefined as a "data" value, is a good choice. Because we often can't have a default value.

Yeah, like I said in the "bug report" I expected as much 😊

Two more points, just to have mentioned them:

  1. If we treat the empty value of a string field as undefined, then the type of it in data needs to be string | undefined. The type that we receive in onSubmit for the submitted value is the same type as the type of data in general. Especially when fields are also marked as required, that can be annoying and/or require extra parsing as part of submit to clarify/prove that the value is the expected shape in the end. This problem is not caused by the type of empty string fields, but having it work like this makes it relevant for more fields
  2. We often want to trim the value in a string field before evaluating presence, and since trimming a string with only spaces results in an empty string (not undefined), that is implicitly also an "empty value" regardless of the default

Either way, we're probably going to fix this in our case by using the default of undefined and then coercing to an empty string before submitting. This was mostly about it being like this by default being unexpected 😊

If you specify [emptyValue=""] and also specify an empty string in defaultData on the form, then the actual value until the field is edited is undefined. This is probably a separate bug, though

About this one, I suppose I should report that as a separate bug.

@tujoworker
Copy link
Member

Thank you for the good feedback 🫶

If you specify [emptyValue=""] and also specify an empty string in defaultData on the form, then the actual value until the field is edited is undefined. This is probably a separate bug, though

Yes, this is a bug. Here is a PR #4111 with an attempt to fix this.

@tujoworker
Copy link
Member

We often want to trim the value in a string field before evaluating presence, and since trimming a string with only spaces results in an empty string (not undefined), that is implicitly also an "empty value" regardless of the default

If you trim simply white spaces, you can use the trim on all string fields.

@tujoworker
Copy link
Member

tujoworker commented Oct 11, 2024

I expected the value of /myPath to be an empty string after editing, not undefined.

I have two suggestions. And I'm not sure what we can or should do. So I would like to get your thoughts about it:

  1. We could "remember" the given "defaultValue" from the data set. For empty strings, it may be just as fine. But what about numbers? or other data types? They need to still fall back to undefined.
  2. You could solve this on your side by using a transformIn on the Form.Handler:
const transformIn = ({ value }) => {
  if (value === undefined) {
    return ''
  }
  return value
}
Screenshot 2024-10-11 at 10 55 47

@tujoworker tujoworker reopened this Oct 11, 2024
tujoworker pushed a commit that referenced this issue Oct 11, 2024
## [10.52.0](v10.51.2...v10.52.0) (2024-10-11)

### 📝 Documentation

* **NationalIdentityNumber:** add docs about validation ([#4077](#4077)) ([51bfd80](51bfd80))
* **OrganizationNumber:** adds docs about validation ([#4078](#4078)) ([c785c51](c785c51))

### 🐛 Bug Fixes

* **Autocomplete:** Show whole suggestion list after item selection ([#4060](#4060)) ([0acf061](0acf061))
* **Field.NationalIdentityNumber:** validate on all digits(not only 11 digits) ([#4079](#4079)) ([7c34fc9](7c34fc9))
* **Field.OrganizationNumber:** should validate on all digits(not only when 9) ([#4071](#4071)) ([08a4b51](08a4b51))
* **Forms:** add support for `Form.SubmitConfirmation` inside Wizard ([#4088](#4088)) ([e1167a4](e1167a4)), closes [#4086](#4086)
* **Forms:** don't call internal `exportValidators` when they not are exported as an array ([#4113](#4113)) ([cd54ed0](cd54ed0)), closes [#4106](#4106)
* **Forms:** ensure `emptyValue` is set in the data context when defined ([#4111](#4111)) ([dcc5694](dcc5694)), closes [#4070](#4070)
* **Forms:** ensure `onBlurValidator` gets called when `validateInitially` is true ([#4069](#4069)) ([59cf6c5](59cf6c5)), closes [#4068](#4068) [#4066](#4066)
* **Forms:** ensure Field.SelectCountry has a fallback locale (nb-NO) ([#4114](#4114)) ([568229a](568229a))
* **Forms:** ensure Form.clearData works in React.StrictMode ([#4053](#4053)) ([da0c93a](da0c93a)), closes [#4048](#4048)
* **Forms:** Fix use of unpolyfilled structuredClone in useData hook ([#4108](#4108)) ([1f59f10](1f59f10))
* **Forms:** keep `Iterate.EditContainer` open when falsy value or empty object was given as the iterate value ([#4087](#4087)) ([e932059](e932059)), closes [#4046](#4046)
* **Forms:** show error on every value change when using exported validators ([#4068](#4068)) ([cab6d01](cab6d01)), closes [#4067](#4067)
* **Icon:** icon property typing should accept FormStatus icons ([#4091](#4091)) ([f49eb34](f49eb34))
* **NationalIdentityNumber:** use `onBlurValidator` instead of `validator` ([#3982](#3982)) ([0a93755](0a93755))
* **Typography:** match medium heading font size in Sbanken theme ([#4039](#4039)) ([ce50529](ce50529))

### ✨ Features

* **Forms:** add `bubbleValidation` to Form.Isolation and Iterate.PushContainer to prevent the form from being submitted when there are fields with errors ([#4103](#4103)) ([880f870](880f870)), closes [#4072](#4072)
* **Forms:** add `createMinimumAgeValidator` to Field.NationalIdentityNumber make a customizable `adultValidator` ([#4057](#4057)) ([6c20ba2](6c20ba2))
* **Forms:** add `Form.useSnapshot` hook to handle snapshots of data ([#4102](#4102)) ([d451793](d451793))
* **Forms:** add `isolatedData` prop to `Iterate.PushContainer` ([#4076](#4076)) ([ede5f8e](ede5f8e))
* **Forms:** add `isValid` to Form.Visibility for showing content based on the validation of a field ([#4038](#4038)) ([7536752](7536752))
* **Forms:** add `Iterate.ItemNo` component ([#4095](#4095)) ([c736c9e](c736c9e))
* **Forms:** add `transformLabel` to all Value.* component ([#4056](#4056)) ([d63e472](d63e472))
* **Forms:** add support for `defaultValue` (and `value`) for fields used in `Iterate.Array` ([#3987](#3987)) ([afbdddf](afbdddf)), closes [#3882](#3882)
* **Forms:** add support for `id` for when using dynamic steps with `activeWhen` ([#4093](#4093)) ([248da92](248da92))
* **Forms:** remove internal pattern from `Field.NationalIdentityNumber` in favor of the internal validator ([#4098](#4098)) ([fb35722](fb35722))
* **Forms:** remove internal pattern from `Field.OrganizationNumber` in favor of the internal validator ([#4092](#4092)) ([e829f8b](e829f8b)), closes [#4073](#4073)
* **Skeleton:** stop animation after 30 seconds ([#3479](#3479)) ([f67b3bb](f67b3bb))
* **Value.PhoneNumber and NumberFormat:** displays country code using prefix `+` instead of `00` ([#4051](#4051)) ([fb363d0](fb363d0))
@tujoworker
Copy link
Member

🎉 This issue has been resolved in version 10.52.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tujoworker
Copy link
Member

Proposal

Introduce fallbackData on the Form.Handler that can contain values that will be used when a field gets undefined.

When used with defaultData , it will be deep merged, but the defaulData will take precedence for the "initial" data.

When emptyValue is set on a field to something else than undefined, then this value will take precedence over fallbackData.

What are your thoughts on such a way of solving the "issue"?

@kimroen
Copy link
Contributor Author

kimroen commented Oct 16, 2024

Thanks for the additional thoughts!

We could "remember" the given "defaultValue" from the data set. For empty strings, it may be just as fine. But what about numbers? or other data types? They need to still fall back to undefined.

I'm not sure I understand how this in turn would relate to defaultData on the form handler 🤔 We use defaultData to pre-fill the form with various data from the backend when someone is resuming filling out a form, so in those cases we don't want them to be the empty value, but I take it that today if you set defaultData the value there will override the defaultValue on the field (I wasn't aware of this property)?

You could solve this on your side by using a transformIn on the Form.Handler

Interesting, I haven't looked much at these transform functions so that could be something to look at for sure 👍

Introduce fallbackData on the Form.Handler that can contain values that will be used when a field gets undefined

Not quite sure what I think about this - the first reaction I have is that I'm not sure this is a better remedy to the unexpected behavior I reported here than just surfacing the default behavior a bit more would be 🤔

  • The word "fallback" here, what is the meaning in this context? If the user hasn't picked anything, then "fall back" to this value? Or more in a technical sense, the "value" in the form will "fall back" to this value when it's not set?
  • What's an example of how this could be used that means fallbackData is better than something closer to the existing "emptyName" naming pattern?
  • How does this relate to "resetting" the form?

I think I'll have to pretend it exists for a moment and try it in our actual form to see if this makes things simpler or not. Gut feeling is that it'll just be repeating the shape of the form data in a new spot, but maybe there's some default behavior or merging I'm not thinking about.

@tujoworker
Copy link
Member

Thank you for all your thoughts and feedback – they’ve been really helpful.

I understand that introducing something new can add complexity, which we want to avoid. Changing the current behavior of defaultData could also lead to unwanted side effects.

If emptyValue and/or defaultValue can address the actual issues where they arise, I believe that's the best approach.

But we still may optimize the outcome of these, such as TypeScript support, like you mentioned:

As a bonus, keeping it a string makes dealing with the types easier 😊

tujoworker pushed a commit that referenced this issue Oct 30, 2024
## [10.54.0](v10.53.0...v10.54.0) (2024-10-30)

### 📝 Documentation

* add documentation about `emptyValue` ([#4174](#4174)) ([52457ba](52457ba)), closes [#4070](#4070)

### 🐛 Bug Fixes

* **Field.Number:** `decimalLimit={0}` when `currency` should work ([#4167](#4167)) ([68123ea](68123ea))
* **Forms:** correct alignment of Wizard status message (error, warning, info) ([#4185](#4185)) ([a307cda](a307cda))
* **Forms:** ensure autocomplete="off" when autoComplete on Form.Handler is false ([#4184](#4184)) ([6795b9f](6795b9f))
* **Forms:** ensure correct sorting in Field.SelectCountry ([#4196](#4196)) ([8db7720](8db7720)), closes [#4195](#4195)
* **Forms:** fix schema validation for required paths with matching name ([#4189](#4189)) ([04caf61](04caf61)), closes [#4179](#4179)
* **Forms:** only run onBlurValidator when no other errors are present ([#4172](#4172)) ([d2797f1](d2797f1)), closes [#4074](#4074)
* **Forms:** render multiple (combined) Ajv errors with translated messages ([#4176](#4176)) ([2b62ef7](2b62ef7)), closes [#4052](#4052)
* **Forms:** updates country names in SelectCountry ([#4187](#4187)) ([bb1b67c](bb1b67c))
* **NumberFormat:** should not throw exception when providing an invalid currency ([#4192](#4192)) ([a0700cd](a0700cd))
* **Table:** fix visible hidden rows for additional expandable table rows ([#4188](#4188)) ([16b6101](16b6101))

### ✨ Features

* add CountryFlag component ([#4181](#4181)) ([76a0c47](76a0c47))
* **Dialog:** add `verticalAlignment` property with `top` alignment support ([#4190](#4190)) ([3ace8b0](3ace8b0))
* **Forms:** add `onDone`, `onCancel` and `onEdit` to Form.Section containers ([#4199](#4199)) ([2f27ad0](2f27ad0))
* **Forms:** add `transformData` to the onSubmit event of Form.Handler ([#4183](#4183)) ([748b604](748b604))
* **Forms:** deprecate `useErrorMessage` hook for when creating your own fields ([#4177](#4177)) ([2ae86f2](2ae86f2)), closes [#4162](#4162)
* **Forms:** deprecate Ajv `validationRule` in FormError and deprecate `errorMessages` keys like `pattern` in favor of Eufemia translation keys like `Field.errorPattern` ([#4162](#4162)) ([b50387a](b50387a))
* **Keyboard navigation:** add support for `key` property (useful for fire events like {ArrowDown} during tests) ([#4182](#4182)) ([cc1ffa8](cc1ffa8))
* **Upload:** `text` and `title` is possible to remove ([#4163](#4163)) ([b6db4a4](b6db4a4))
* **Upload:** removes subtitle displaying file extension ([#4156](#4156)) ([9bf9b9e](9bf9b9e))
* **Upload:** updates drag and drop texts in Norwegian ([#4170](#4170)) ([ad5bbec](ad5bbec))
@tujoworker
Copy link
Member

🎉 This issue has been resolved in version 10.54.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants