react-forms New conditions handler (defined globally for schema instead of locally for field)

My proposal for a new conditions handler:

conditions are defined "globally" instead of "locally" per field
field listeners are registered only for fields involved in "when-clauses"
the relevant conditions are added as custom data for each field listener (field.data.conditions)
when conditions triggers, the resulting ui-effects are added to uiState for affected fields via uiStateReducer
when conditions de-driggers, the ui-effects are removed
uiState is available in useFormApi(). Any component can check the uiState to decide how/if it should render
set-instructions in conditions are "one-shot". I.e. when the new field value is set, the set instruction is removed.

TODO:

TypeScript (I don't know TS, so I'm not the right man to add typing)
Backwards compability for old conditions structure (should probably be handled at the top of RegisterConditions).
My conditionParser differs some from the old one. I put mine in conditions2.js in order to not break other stuff.
uiStateReducer originally user Immer in my code. I took some shortcuts with the immutability when I removed Immer. This should probably be addressed.
Documentation
Decide how to use uiState (hiding and disabling fields etc)
Circular conditions are possible. It's kind of hard to walk into this problem, but it should probably be handled some way.

I'm sure there are lots to discuss before this reaches production.

Jul 15 '20 21:07 msageryd

So the rendering performance seems solid. Looking at the profiler, the only time when multiple fields are re-rendered in the same cycle is when a condition is met/broken which makes sense and has to happen. :+1: on that.

I will now dig a bit deeper trough the code.

Jul 17 '20 06:07 Hyperkid123

DONE:

uiStateReducer originally user Immer in my code. I took some shortcuts with the immutability when I removed Immer. This should probably be addressed. Fixed in commit 12ce210
Circular conditions are possible. It's kind of hard to walk into this problem, but it should probably be handled some way. I cannot reproduce my circular scenario anymore. I'm guessing that this problem is solved due to the better immutability handling in uiStateReducer.

Jul 17 '20 20:07 msageryd

DONE (98d2064):

My conditionParser differs some from the old one. I put mine in conditions2.js in order to not break other stuff. conditions2.js has now replaced the old conditions.js. This will break legacy condition definitions. A future legacyConditionsMapper will convert old definitions to the new structure and un-break this
Decide how to use uiState (hiding and disabling fields etc) I decided that FormFieldConditionWrapper should be removed. The condition data is already available in uiState in context and used as input to FormFieldHideWrapper

Jul 17 '20 21:07 msageryd

@Hyperkid123 Some design questions:

Do you want the conditions collection to be an object or an array?

Object pros:

Forces user to define names for each condition.
Easier to debug with good names.
Easier to merge multiple conditions objects

Array pros:

Persisted order of conditions

Is support for sequence still needed?

You said earlier that you wanted to get rid of sequence conditions. Does this mean that the legacyConditionsMapper does not have to cover sequences?

Jul 17 '20 21:07 msageryd

@Hyperkid123 I can't find any repo for the documentation. Thought I'd update the docs for the new conditions schema.

Jul 18 '20 12:07 msageryd

@Hyperkid123 I've implemented a naive legacyConditionsMapper. It can only handle simple visibility conditions for now. This needs to be revisited in order to handle more complex conditions. f1818eb

Jul 18 '20 13:07 msageryd

@Hyperkid123 I have started to update the tests for conditions. How do I run the tests? yarn test seems to run the tests, but all test fails, so I suppose I need to do something more.

Jul 18 '20 15:07 msageryd

@msageryd sorry for the silence (weekend). Going through the changes now.

Jul 20 '20 07:07 Hyperkid123

I decided that FormFieldConditionWrapper should be removed. The condition data is already available in uiState in context and used as input to FormFieldHideWrapper

This might cause issues with the value initialization and submission. There is a difference between the condition wrapper and the hidden wrapper.

If the condition is not met the field is not in the DOM and once the condition is met you can trigger mounting events like re-initialize the field. So if the condition is false the value is not sent to the submit callback. Yes value of the condition can be persisted even if it's hidden but that is aa opt-in attribute.

If the hideField is true, the filed is not visible but still present in the DOM and its value is submitted. It is basically an input of type hidden for non-input fields.

So the condition wrapper might no longer be required but we still need to detach/attach the field from/to the DOM.

Jul 20 '20 08:07 Hyperkid123

Do you want the conditions collection to be an object or an array?

From the development point of view, the object is preferable. But the order of execution is important and it makes it a bit easier for a user to understand and define the condition order. That is ultimately the reason why the whole schema is a nested array since its sometimes created by users who do not necessarily have any experience with any development. I would like to stick with the Array if possible.

Is support for sequence still needed?

You said earlier that you wanted to get rid of sequence conditions. Does this mean that the legacyConditionsMapper does not have to cover sequences?

Sequences still have to be supported. We have to keep them at least until the next major release. We introduced them to ensure the correct order of execution of conditions using AND and OR statements when we introduced the set outcome of a condition.

Jul 20 '20 08:07 Hyperkid123

@Hyperkid123 I can't find any repo for the documentation. Thought I'd update the docs for the new conditions schema.

its the react-renderer-demo package (I know what an intuitive name for a docs package). https://data-driven-forms.org/development-setup#rundocumentation

I would like to add that if run locally, the docs use locally build packages so it is required that all packages are built on your local machine. Running yarn build in the root directly would do that for you.

Jul 20 '20 08:07 Hyperkid123

@Hyperkid123 I have started to update the tests for conditions. How do I run the tests? yarn test seems to run the tests, but all test fails, so I suppose I need to do something more.

There is the same requirement as with the docs. All packages must be built locally in since the tests use a local build of all packages. This is there to ensure that no matter what changes in what packages you make, it will run all the tests with all of these changes. If you have all packages build already and you still see some error, just post the stack trace here and we can look at it.

But looking at the circle CI log, you have a circular dependency so the renderer package wont even build:

./src/index.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/component-types.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/compose-validators.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/conditions-mapper.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/data-types.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/default-schema-validator.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/field-array.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/field-provider.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/form-renderer.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/form-spy.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/form.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/legacy-conditions.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/register-conditions.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/renderer-context.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/schema-errors.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/set-field-values.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/ui-state-reducer.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/use-field-api.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/use-form-api.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/validator-mapper.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/validator-types.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/validators.js, /home/circleci/react-forms/packages/react-form-renderer/src/files/wizard-context.js → ./dist/cjs...
 
[!] Error: Circular dependency: src/index.js -> src/files/form-renderer.js -> src/files/register-conditions.js -> src/index.js
 
Error: Circular dependency: src/index.js -> src/files/form-renderer.js -> src/files/register-conditions.js -> src/index.js
 
    at Object.onwarn (/home/circleci/react-forms/packages/react-form-renderer/rollup.config.js:43:9)
 
    at Object.config.onwarn.warning [as onwarn] (/home/circleci/react-forms/node_modules/rollup/dist/shared/node-entry.js:13599:25)
 
    at Graph.config.onwarn.warning [as onwarn] (/home/circleci/react-forms/node_modules/rollup/dist/shared/node-entry.js:13599:25)
 
    at Graph.warn (/home/circleci/react-forms/node_modules/rollup/dist/shared/node-entry.js:13403:14)
 
    at Graph.link (/home/circleci/react-forms/node_modules/rollup/dist/shared/node-entry.js:13420:18)
 
    at Promise.all.then (/home/circleci/react-forms/node_modules/rollup/dist/shared/node-entry.js:13286:18)
 
    at process._tickCallback (internal/process/next_tick.js:68:7)

That may be the reason why the tests fail.

Jul 20 '20 08:07 Hyperkid123