Design decision

[msageryd]

Hi.
Thank you for building this really nice form renderer. I'm curious about the design decision to give the possibility put "condition" in every "field". I can only think of one argument to have a condition tied to a specific field, to set the visibility och this very field without having to appoint the field by name. I might have misunderstood something..

A setter can set values for any named field, regardless of which is the active field (i.e. regardless of which field the condition belongs to). Actually, the setter has to state the name of the field to set its value, so there is no use for the knowledge of which field the condition belong to.

With many conditions it can be a bit messy. I wonder if you contemplated to put all conditions in one big form level condition object instead of spreading conditions among the fields? This condition object could set visibility for named fields, much like values can be set for named fields.

I'm not suggesting a rebuild of your condition engine. I just want to understand it better.

I mean like this:

formOptions: {
  conditions: [{
    when: 'field1',
    is: '10',
    then: {
      show: 'field2',
      set: {
        field3: 'new value for field 3'
      }
    }
  },
  {
    when: 'fieldX',
    is: '99',
    then: {
      hide: 'fieldY'
    }
  }]
}

[Hyperkid123]

Hello @msageryd We had a myriad of long discussions about the condition definitions and to be honest we are not exactly happy with the current implementation either. It would deserve a proper revisit.

Unfortunately, we are talking about breaking change which will impact probably all of our users. So if we want to incorporate change like that to v2.x, we still need to support the current format. And we do not plant v3 release at this time. So this would have to be a separate feature. That said we always appreciate fresh pair of eyes to look over the project. So any suggestion and potential collaboration are more than welcome.

I can only think of one argument to have a condition tied to a specific field, to set the visibility och this very field without having to appoint the field by name.

That is exactly how the condition was originally used. It was only for dynamically showing/hiding certain fields. The ability to set value was added later and we had to maintain compatibility of the schema. So that is the main and really only reason why the definition is like this. To preserve backwards compatibility until we will move forward with next major release.

I am not against having a global condition definition. It does make sense to me. We could introduce a new rule to the schema validation to preserve the functionality without breaking changes. Of there is a global condition object defined in the schema, fields can't have their own declaration. That would allow us to keep current implementation and start working on something more sophisticated unit we are ready to release a new major version of the renderer.

@rvsia do you have anything to add?

[msageryd]

If you want to move towards a central condition object you could have a compability function which collects all field level conditions and puts them in the central condition object. The only thing you'd need to add is the fieldName for visibility conditions (since those are without fieldNames in current solution).

[Hyperkid123]

If you want to move towards a central condition object you could have a compability function which collects all field level conditions and puts them in the central condition object. The only thing you'd need to add is the fieldName for visibility conditions (since those are without fieldNames in current solution).

Yes, that would be the next step. Once we have a solid solution to the global condition config. We would have to deal with potential conflicts between the global and field level definitions.

[rvsia]

@msageryd Thanks for suggesting this! We have discussed this feature some time ago so yeah, I agree with introducing some kind of 'lifecycle' condition that is computed after each change. We just need to be extra careful in terms of performance, but I think it should be manageable.

Having hide, show commands seems reasonable to me. I would just enhance it with having an array of names as the second option:

hide/show: 'field1'
// or
hide/show: ['field1', 'field2']

Also, to get cleaner syntax, we should get rid of and and or shorthands in these global conditions. (So we will be able to get rid of the sequence type too.)

[msageryd]

How about maintaining a list modifiedSinceLastConditionCheck by subscribing to all changed values. This list could be checked against all fields involved in condition-checks. This way you could run only the necessary condition checks.

It would be cool if final-form had such a list which would reset at a manual checkpoint. Maybe form.modifiedSinceLastCheckpoint, accompanied by a function for creating a checkpoint. createModificationCheckpoint() would be called by react-forms after each condition check to reset the list.

[msageryd]

No, scratch that idea. I'm now fiddling with the following:

• Traverse the conditions object
• Whenever a key named when is encountered the fieldName (or fieldNames if it's an array) will be used as keys in a map where each item holds an array of conditions related to this particular field.

The resulting map (or object) will have one key for every "test-triggering" fieldName. Since each condition can be triggered by many fields, the same condition can be referenced from more than one place in the map.

The map will be a speedy lookup for whenever a field is changed. The only condition tests that need to be ran are those in the array for the particular field.

I'm building this for myself right now. Just tell me if you are interested in the code.

Any thoughts?

[Hyperkid123]

@msageryd feel free to open PR. We always appreciate any improvents. I wont have more time today to discuss but i will take a look tommorow as soon as i can.

[Hyperkid123]

@msageryd did you have any time to publish the example? I am very interested in your proposition.

[msageryd]

I actually went in another direction. Ended up just using your elegant condition parser. I had too much special requirements, so I built a quite different form engine.

But I do think that the core of my condition handler would fit nicely with data-drive-forms. It would be great if I could
contribute since I have had great use for your parser.

Before porting my condition handler I would like to discuss some details so I can get is as much right as possible.

Some differences between my code and your code:

• I'm building for React Native, but I don't think anything will differ due to the platform
• I'm using Immer. It's only in one place (reducer), so it should be easy to switch to plain js
• All my input JSON is all object, i,e. no arrays. This is because my forms needs to be easily mergeable.
• I create my form-object in an effect, so my form object is available even before the render function. This is needed in my solution, but I don't think you want that in data-driven-forms. I think I can repackage this into a component.

The concept for my condition handler:

1. conditions are defined globally (i.e. not per field)

2. At first render I traverse all conditions and restructure them to a "per-affecting-field" structure.
   This gives me a list of fields which are then created as field listeners. I.e. all fields in all when-clauses
   will have a listener.

3. When a field listener find out that a condition is met the then or else clause is stored in an object
   which is keyed with the affected fieldName. This gives a speedy way to check if a field is currently affected by a
   condition. This object is called uiState and is made available via context.

uiState actually hold an array (stack) of active conditions per fieldName. This is done because one field can be affected by more than one condition at the same time. The lastly added condition is the "current". If this condition stops being met, it will be removed from the stack and the next item will automatically be the current.

The code for a Field component might look like this:

  const {uiState} = useFormApi();

  const itemState = uiState.fields[item.name] : uiState;
  const hidden = itemState?.visible?.[0]?.value === false;
  if (hidden) return null;

  const disabled = itemState?.disabled?.[0]?.value;

I think that data-driven-forms would need a helper function for this, since it's not very straight forward. The "current" active condition is always the first in the stack [0].

4. The condition stacks are kept up to date via a reducer. Each field listener gets a dispatch function when it's created, so it's easy for the listeners to communicate up in the tree by simply dispatching the result from the condition parser.

I realise that I'm quite bad at trying to explain what I do. The code will probably tell the story a litle better.

This is how my conditions section looks like in the form definition:

conditions: {
  caveatNotesNotEmpty: {
    when: 'caveatNotes',
    isNotEmpty: true,
    then: {
      fields: {
        caveatNotes: {
          icon: 'warning',
        },
        anotherField: {
          disabled: true,
        },
        yetAnotherField: {
          set: 'New value for this field',
          visible: true
        }
      },
    },
  },
  companyNameIsABC: {
    when: 'companyName',
    is: 'ABC',
    then: {
      fields: {
        companyAddress: {
          visible: true
        },
      },
    },
  },
}
```

Would you like me to try to port my condition handler and make a PR? I only work in React Native and have never used React. But I suppose it's much the same. I only have to figure out how to spin up a React server so I can test my code in a clone of data-driven-forms.

[Hyperkid123]

@msageryd

Would you like me to try to port my condition handler and make a PR?

Definitely yes, I think it could be a nice improvement. As I stated before, the only condition would be keeping backward compatibility of the schema and no breaking changes of the useFieldApi hook. Other than that you can do whatever you want.

To address some of your points.

I'm building for React Native, but I don't think anything will differ due to the platform

There should be no problems with any logic. I've found that the biggest issue when porting from React native to React and vice verse is the difference in styling. So we should be good here.

I'm using Immer. It's only in one place (reducer), so it should be easy to switch to plain js

Yup just another implementation of "reducer"

All my input JSON is all object, i,e. no arrays. This is because my forms needs to be easily mergeable.

I can understand that. We actually have an unreleased package with schema parsers. We used to do a lot of conversion between a number of different schema types into the DDF schema we use now.

I create my form-object in an effect, so my form object is available even before the render function. This is needed in my solution, but I don't think you want that in data-driven-forms. I think I can repackage this into a component.

Agreed on this point.

conditions are defined globally (i.e. not per field)

This will require traversal of the schema before render and extracting all conditions into such a global definition. You can use the schema TS definition to help you navigate through it. We do traverse schema already for validation purposes to avoid potential runtime error if invalid schema is passed.

I think that data-driven-forms would need a helper function for this, since it's not very straight forward. The "current" active condition is always the first in the stack [0].

Yes, we evaluate them in order.

I can't wait for the PR.
🏎️

[msageryd]

I tried to get going with this, but I'm running into trouble. Mind you, I've never even started a React app. But it doesn't seem too hard, according to your docs https://github.com/msageryd/react-forms/blob/master/packages/react-form-renderer/README.md#development-setup.

yarn install works fine
yarn build works fine (after nvm i 10 to get the latest Node 10)
yarn start won't work (I tried both MUI and PF3)

[Hyperkid123]

@msageryd did you build all packages or just one? Running build in root is required before starting development. We allow building packages separately to make it easier to test our builds.

We should explicitly state that in docs.

[msageryd]

yarn install and yarn build was executed in the root (the react-forms folder)

[rvsia]

@msageryd Can you try to run yarn start in react-form-renderer folder? This project does not have any other dependencies, so it should work (or it should prove that there is something wrong). If this works, please run again yarn build in that same folder and then try to run other packages.

[msageryd]

@rvsia Thanks, that worked.

There is a PR waiting to be checked out. The code lives in a branch "new-conditions-structure". I altered index.js in react-form-renderer so it shows some conditions examples.

[msageryd]

Hmm. I just discovered that my uiStateReducer is buggy. This code was originally quite different (bult with Immer). I'll need to go through it some more to make it work as intended. There is a problem with removing old uiStates.

You will be able to see the guts of the proposed solution anyway. If you find it interesting, I'll go ahead and fix the bug.

[Hyperkid123]

I will try to check it out today. If I don't give any feedback, don't hesitate to remind me (lot of stuff is going on atm.).

[msageryd]

I've fixed the reducer bug. I also made the sample form more to the point.

[Hyperkid123]

Looking at the code right now.