fix: ui:emptyValue behavior #2457

ssbyoung · 2021-07-01T19:16:13Z

Please note that this PR relies on #2456 being merged and released

Reasons for making this change

By default, if a form has a field with an empty string and ui:emptyValue is filled out, the empty value will not appear until the user interacts with the form. I believe that this is incorrect. This change makes it so that ui:emptyValue will be populated on the first form render.

I also refactored everywhere this logic lived to use a hook inside of @rjsf/core. Because it is a hook it requires the user to be on at least react 16.8.0, hence the need for #2456 . I also updated all the peer dependencies to 3.0.0. If PR #2456 goes through as a major version. All of these files will need to be updated.

Important Changes

ui:emptyValue will be populated for empty form fields
If ui:emptyValue is not populated AND the user enters the empty string onChange will be called with the empty string instead of undefined.

Checklist

I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed

ssbyoung · 2021-07-01T19:19:31Z

packages/core/index.d.ts

@@ -286,6 +288,22 @@ declare module '@rjsf/core' {

    export module utils {

+        type UseEmptyValueOnChangeProps = {


I'm definitely not super strong at typescript. If someone has a suggestion how to clean this up, it would be much appreciated.

ssbyoung · 2021-07-01T19:19:51Z

packages/core/package.json

@@ -14,7 +14,7 @@
    "precommit": "lint-staged",
    "publish-to-npm": "npm run build && npm publish",
    "start": "concurrently \"npm:build:* -- --watch\"",
-    "tdd": "cross-env NODE_ENV=test mocha --require @babel/register --watch --require ./test/setup-jsdom.js test/**/*_test.js",
+    "tdd": "cross-env BABEL_ENV=test NODE_ENV=test mocha --require @babel/register --watch --require ./test/setup-jsdom.js test/**/*_test.js",


This command does not work without the BABEL_ENV being set.

ssbyoung · 2021-07-01T19:20:32Z

packages/core/test/StringField_test.js

@@ -208,6 +208,21 @@ describe("StringField", () => {
      });
    });

+    it("should render ui:EmptyValue when formData is emptyString and ui:EmptyValue is defined", () => {


This is the most important test in this PR. The act(() => {}) gives the chance for the hook to run.

ssbyoung · 2021-07-01T19:21:07Z

packages/core/test/StringField_test.js

@@ -221,7 +236,7 @@ describe("StringField", () => {
      });

      sinon.assert.calledWithMatch(onChange.lastCall, {
-        formData: undefined,
+        formData: "",


These were previously undefined because options.emptyValue was undefined. If the user enters the empty string, I believe we should call onChange with the empty string.

ssbyoung · 2021-07-01T19:21:55Z

packages/core/src/utils.js

@@ -7,6 +7,11 @@ import jsonpointer from "jsonpointer";
 import fields from "./components/fields";
 import widgets from "./components/widgets";
 import validateFormData, { isValid } from "./validate";
+import useEmptyValueOnChange from './hooks/useEmptyValueOnChange';
+
+export const hooks = {


I just tacked the hooks onto the utils. If there is a better place, let me know.

ssbyoung · 2021-07-01T19:22:23Z

packages/core/src/hooks/useEmptyValueOnChange.js

+    return rawOnChange(newValue);
+  }, [rawOnChange]);
+
+  useEffect(() => {


This useEffect takes care of the inital render state.

ssbyoung · 2021-07-01T19:22:54Z

packages/core/src/hooks/useEmptyValueOnChange.js

+
+  useEffect(() => {
+    if (value === mergeValues(value)){
+      return;


Added in this guard clause because I was seeing tests fail for onChange getting called multiple times if the value wasn't empty.

epicfaace · 2021-07-12T21:42:21Z

By default, if a form has a field with an empty string and ui:emptyValue is filled out, the empty value will not appear until the user interacts with the form. I believe that this is incorrect. This change makes it so that ui:emptyValue will be populated on the first form render.

Trying to understand why we need to change this though -- isn't populating data on the first form render what default in the schema is for? Why couldn't someone just set something equal to ui:emptyValue and default if they want this behavior?

I think that when ui:emptyValue was originally added to this library, the intention was for it to only override the default behavior of setting a field to undefined after the user interacts with the form -- "When a text input is empty, the field in form data is set to undefined" (see https://github.com/rjsf-team/react-jsonschema-form/blob/3ec17f1c0ff40401b7a99c5e9891ac2834a1e73f/docs/usage/validation.md#the-case-of-empty-strings).

Maybe could you further explain your use case so we can see if this PR is the best solution or there is another alternative that may work better?

ssbyoung · 2021-07-13T15:24:00Z

@epicfaace default value does not work properly. Here is a a small example demonstrating it:
rjsf-default-issue.zip

I think default is the behavior I want. It wasn't documented and I only found the syntax in the playground.

My use case is that when I initially render the form with empty data, I'd like to have certain fields have pre-populated values as defined in the schema.

epicfaace · 2021-07-13T15:32:07Z

@ssbyoung can you share the issue with default using a playground example? https://rjsf-team.github.io/react-jsonschema-form/

ssbyoung · 2021-07-13T15:38:41Z

@epicfaace Can you take a look at the example I sent? I think there might be an issue with the consumer of this library using react 17 and this library using react 16. I'm also unsure how to modify the playground with the issue I'm seeing.

epicfaace · 2021-07-13T15:51:16Z

I think you could make a codepen here (https://codepen.io/epicfaace/pen/WNjvBrb) and put in your desired versions of React / rjsf

ssbyoung · 2021-07-13T16:03:21Z

@epicfaace I'm having a hard time getting a codepen with [email protected]. Do you have an example with one that uses the latest version?

ssbyoung · 2021-07-13T16:08:25Z

@epicfaace is the playground https://rjsf-team.github.io/react-jsonschema-form/ running on version 3.0.0?

epicfaace · 2021-07-13T16:33:53Z

Oh, I think codepens with v3 an issue because of #2447. Could you try using a codepen with the latest version of v2 instead? I'm guessing the issue will also appear in that version.

ssbyoung · 2021-07-13T17:02:19Z

@ssbyoung https://codepen.io/ssbyoung/pen/NWjdNjv?editors=1111 is a codepen showing this same issue.

epicfaace · 2021-07-13T17:06:58Z

Hmm -- why is the original formData set to { prop1: undefined }? If you just set formData to {}, then the default properly gets set.

…

-- Ashwin Ramaswami

On Tue, Jul 13, 2021 at 1:02 PM Broderick Young ***@***.***> wrote: @ssbyoung <https://github.com/ssbyoung> https://codepen.io/ssbyoung/pen/NWjdNjv?editors=1111 is a codepen showing this same issue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2457 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM4MX4AXAK5KZ2MREQPYXTTXRWSNANCNFSM47VKA6QQ> .

ssbyoung · 2021-07-13T17:21:46Z

As a consumer I'd expect all falsey values to to get replaced. Does it explicitly not have to be set in the object for the default to appear?

ssbyoung · 2021-07-13T18:00:24Z

@epicfaace after trying default with my code, the current implementation works as is. I will be closing this PR as well as the other one related to this change.

ssbyoung added 6 commits July 1, 2021 11:06

fix for core

a27961b

do semantic ui

3cbb849

do antd

a7bee3e

do bootstrap-4

e719f45

do fluent-ui

a3a76f5

do material-ui

7edc0c6

ssbyoung requested a review from agustin107 as a code owner July 1, 2021 19:16

ssbyoung changed the title ~~Ssbyoung/fix empty value~~ fix: ui:emptyValue behavior Jul 1, 2021

ssbyoung commented Jul 1, 2021

View reviewed changes

ssbyoung mentioned this pull request Jul 1, 2021

ui:emptyValue does not work when initial data has an empty string #2449

Closed

2 tasks

lint fixes.

0239989

epicfaace added the breaking change label Jul 12, 2021

ssbyoung closed this Jul 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ui:emptyValue behavior #2457

fix: ui:emptyValue behavior #2457

ssbyoung commented Jul 1, 2021 •

edited

Loading

ssbyoung Jul 1, 2021

ssbyoung Jul 1, 2021

ssbyoung Jul 1, 2021

ssbyoung Jul 1, 2021

ssbyoung Jul 1, 2021

ssbyoung Jul 1, 2021

ssbyoung Jul 1, 2021

epicfaace commented Jul 12, 2021

ssbyoung commented Jul 13, 2021

epicfaace commented Jul 13, 2021

ssbyoung commented Jul 13, 2021

epicfaace commented Jul 13, 2021

ssbyoung commented Jul 13, 2021

ssbyoung commented Jul 13, 2021

epicfaace commented Jul 13, 2021

ssbyoung commented Jul 13, 2021

epicfaace commented Jul 13, 2021 via email

ssbyoung commented Jul 13, 2021

ssbyoung commented Jul 13, 2021

		@@ -286,6 +288,22 @@ declare module '@rjsf/core' {

		export module utils {

		type UseEmptyValueOnChangeProps = {

fix: ui:emptyValue behavior #2457

fix: ui:emptyValue behavior #2457

Conversation

ssbyoung commented Jul 1, 2021 • edited Loading

Reasons for making this change

Important Changes

Checklist

ssbyoung Jul 1, 2021

Choose a reason for hiding this comment

ssbyoung Jul 1, 2021

Choose a reason for hiding this comment

ssbyoung Jul 1, 2021

Choose a reason for hiding this comment

ssbyoung Jul 1, 2021

Choose a reason for hiding this comment

ssbyoung Jul 1, 2021

Choose a reason for hiding this comment

ssbyoung Jul 1, 2021

Choose a reason for hiding this comment

ssbyoung Jul 1, 2021

Choose a reason for hiding this comment

epicfaace commented Jul 12, 2021

ssbyoung commented Jul 13, 2021

epicfaace commented Jul 13, 2021

ssbyoung commented Jul 13, 2021

epicfaace commented Jul 13, 2021

ssbyoung commented Jul 13, 2021

ssbyoung commented Jul 13, 2021

epicfaace commented Jul 13, 2021

ssbyoung commented Jul 13, 2021

epicfaace commented Jul 13, 2021 via email

ssbyoung commented Jul 13, 2021

ssbyoung commented Jul 13, 2021

ssbyoung commented Jul 1, 2021 •

edited

Loading