Support for if, else, then rules #2506

Juansasa · 2021-08-06T13:10:52Z

Added if then else logic to resolve schemas

Reasons for making this change

If else, then support is crucial for doing dynamic form while avoiding lots of complicated logics. Forms will be much easier to generate as well.

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
I'm adding a new feature
- I've updated the playground with an example use of the feature

Added if then else logic to resolve schemas

Juansasa · 2021-08-06T15:04:54Z

Inspired by #2466 but without other unrelated fixes

levyitay · 2021-08-06T17:58:46Z

Looks good please merge

levyitay

Looks good to me, only 1 minor change in the method's name

packages/core/src/utils.js

nickgros · 2021-08-11T12:55:22Z

I don't know if I would say the other changes in #2466 are "unrelated". At first glance, this implementation can't handle $refs in if/then/else, e.g. this test.

Juansasa · 2021-08-13T09:36:47Z

I don't know if I would say the other changes in #2466 are "unrelated". At first glance, this implementation can't handle $refs in if/then/else, e.g. this test.

I'd say optimizations and other

I don't know if I would say the other changes in #2466 are "unrelated". At first glance, this implementation can't handle $refs in if/then/else, e.g. this test.

You are correct abut the $ref issue, new test added.
What i mean with unrelated is the other refactoring/improvements to validation which i think is non-essential for resolving ifThen schemas and should belong in a separate PR.

But i might be wrong. My intention is to make a minimal change-set to get this approved as quickly as possible as we are already using this on a separate project. Sorry if i sidestep you on this

nickgros · 2021-08-13T13:26:57Z

What i mean with unrelated is the other refactoring/improvements to validation which i think is non-essential for resolving ifThen schemas and should belong in a separate PR.

I totally get it-- #2466 inherited a lot of cruft from #1666 (for instance, that #1666 dropped json-schema-merge-allof in favor of a custom solution, but it looks like you're using it with no problems).

My use cases anticipate complex schemas, which is why I didn't decouple optimizations from that PR. I think some form of optimization will still be necessary if this PR, and not #2466, is merged.

epicfaace · 2021-08-13T13:36:56Z

Thanks for both your PRs @Juansasa and @nickgros ! I don't really have the bandwidth to compare implementations at the moment, would someone be able to carefully review both PRs and recommend the best plan of action (i.e., which one to merge, or if both PRs should be combined in some way)?

Ultimately, I'd like to have a PR merged that has the most extensive if-else-then support, and probably we want to do that in the simplest way with the least changes.

AlimovSV · 2021-08-24T07:34:22Z

Any updates? We are looking forward to if-then-else implementation will be released.

jmsaulnier · 2021-10-29T18:55:04Z

Thanks for both your PRs @Juansasa and @nickgros ! I don't really have the bandwidth to compare implementations at the moment, would someone be able to carefully review both PRs and recommend the best plan of action (i.e., which one to merge, or if both PRs should be combined in some way)?

Ultimately, I'd like to have a PR merged that has the most extensive if-else-then support, and probably we want to do that in the simplest way with the least changes.

any updates? Thanks 🙏

heath-freenome · 2021-11-17T21:08:18Z

Thanks for both your PRs @Juansasa and @nickgros ! I don't really have the bandwidth to compare implementations at the moment, would someone be able to carefully review both PRs and recommend the best plan of action (i.e., which one to merge, or if both PRs should be combined in some way)?

Ultimately, I'd like to have a PR merged that has the most extensive if-else-then support, and probably we want to do that in the simplest way with the least changes.

@epicfaace why not merge the simpler implementation then @nickgros can rebase his changes on top and deal with the corner cases? At least then there will be some movement on this needed feature.

epicfaace · 2021-11-18T01:49:37Z

packages/core/src/utils.js

@@ -662,12 +662,40 @@ export function stubExistingAdditionalProperties(
  return schema;
 }

+const resolveCondition = (schema, rootSchema, formdata) => {


Rename formdata to formData

Add a comment describing what this function does

epicfaace · 2021-11-18T01:50:08Z

@epicfaace why not merge the simpler implementation

Good idea! I took a quick pass, I could use some help reviewing. Don't 100% understand how resolveCondition works / whether its implementation is correct

epicfaace · 2021-11-18T01:51:37Z

@nickgros any problems with that approach? is it possible that this PR may not be backwards compatible with #2466?

nickgros · 2021-11-18T03:39:56Z

@nickgros any problems with that approach? is it possible that this PR may not be backwards compatible with #2466?

This is fine with me. Many of the changes I made in #2466 are performance optimizations that I needed for my use case (I need to support very large/complex schemas with conditions). The changes I made may not necessarily even be the right approach to solve those problems.

Since this PR is more straightforward and seems to enable this (very valuable!) feature, I think it's worth moving forward for the sake of improving this library. Optimizations can always come later.

packages/core/test/utils_test.js

packages/playground/src/samples/index.js

jmsaulnier · 2021-12-08T18:43:50Z

Any updates on merging this PR? @Juansasa @nickgros @epicfaace
Thanks!

ksbrar · 2021-12-22T00:40:11Z

Also eagerly awaiting this feature!

dieseldjango · 2022-02-09T22:28:12Z

How is this coming? Anything I can help with?

heath-freenome · 2022-02-09T22:55:16Z

@Juansasa consider joining the rjsf weekly meeting on 2/11 to discuss your PR... See #2677 for details on how to join

nickgros · 2022-02-10T19:10:17Z

In #2700, I added changes based on reviews on this PR, and also added the integration tests that I wrote in #2466. We could then look at reviewing/merging #2700 in the weekly meeting

epicfaace · 2022-02-10T20:12:57Z

Great. Please add to the agenda! Ashwin Ramaswami

…

On Thu, Feb 10, 2022, 2:10 PM Nick Grosenbacher ***@***.***> wrote: In #2700 <#2700>, I added changes based on reviews on this PR, and also added the integration tests that I wrote in #2466 <#2466>. We could then look at reviewing/merging #2700 <#2700> in the weekly meeting — Reply to this email directly, view it on GitHub <#2506 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM4MX3LAIGZA7VBDPT4EZDU2QESLANCNFSM5BV4KLHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Juansasa · 2022-02-15T15:33:21Z

Sorry guys missed the notifications.
I fixed the tests and a bug based on our internal usage.
Let me know if you guys need further assistance

nickgros · 2022-02-18T15:05:01Z

packages/core/src/utils.js

+        resolvedSchemaLessConditional,
+        retrieveSchema(conditionalSchema, rootSchema, formdata)


Noticed that this was swapped (presumably to fix a bug?). Is there a test case we could add that would catch a regression?

{ if: ..., then: {minLength: 5}, minLength: 10 }

Expected behavior: inner property (minLength: 5) should override outer property (minLength: 10). We should add a test case for this

nickgros · 2022-02-18T15:05:16Z

packages/core/test/utils_test.js

+          required: ["animal", "food"],
+        });
+      });
+      it.only("should resolve $ref", () => {


remove .only

epicfaace · 2022-02-18T16:46:27Z

Thanks for your help @Juansasa !!! 🎉 @nickgros will take a couple of tiny changes forward in #2700 and we will make sure you get credit when the PR is merged.

Added tests

f72a62c

Added if then else logic to resolve schemas

Juansasa mentioned this pull request Aug 6, 2021

Multi dependence dynamic form #2493

Closed

2 tasks

levyitay suggested changes Aug 9, 2021

View reviewed changes

packages/core/src/utils.js Outdated Show resolved Hide resolved

Changed resolve method's name

75c4d84

levyitay approved these changes Aug 9, 2021

View reviewed changes

epicfaace self-requested a review August 11, 2021 01:37

Added $ref tests

a08c563

Juansasa force-pushed the if_then_else branch 3 times, most recently from 8a90e24 to a08c563 Compare August 18, 2021 08:21

epicfaace reviewed Nov 18, 2021

View reviewed changes

nickgros reviewed Nov 18, 2021

View reviewed changes

packages/core/test/utils_test.js Outdated Show resolved Hide resolved

nickgros reviewed Nov 18, 2021

View reviewed changes

packages/playground/src/samples/index.js Outdated Show resolved Hide resolved

dylanfprice mentioned this pull request Dec 16, 2021

Show conditional field when checked boxes includes some options? #2625

Closed

3 tasks

This was referenced Feb 10, 2022

Optimizations to improve performance on complex conditional schemas #2466

Closed

If/Then/Else - Code Review changes from #2506 and tests from #2466 #2700

Merged

Juansasa and others added 3 commits February 15, 2022 11:44

Fixed merge order

5ccf7ec

Update tests method names

17816d3

Merge branch 'master' into if_then_else

fa0269a

nickgros reviewed Feb 18, 2022

View reviewed changes

packages/core/test/utils_test.js

required: ["animal", "food"],

});

});

it.only("should resolve $ref", () => {

Copy link

Contributor

nickgros Feb 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove .only

epicfaace closed this Feb 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for if, else, then rules #2506

Support for if, else, then rules #2506

Juansasa commented Aug 6, 2021 •

edited

Loading

Juansasa commented Aug 6, 2021

levyitay commented Aug 6, 2021

levyitay left a comment

nickgros commented Aug 11, 2021

Juansasa commented Aug 13, 2021 •

edited

Loading

nickgros commented Aug 13, 2021

epicfaace commented Aug 13, 2021

AlimovSV commented Aug 24, 2021

jmsaulnier commented Oct 29, 2021

heath-freenome commented Nov 17, 2021 •

edited

Loading

epicfaace Nov 18, 2021

epicfaace Nov 18, 2021

epicfaace commented Nov 18, 2021 •

edited

Loading

epicfaace commented Nov 18, 2021

nickgros commented Nov 18, 2021

jmsaulnier commented Dec 8, 2021

ksbrar commented Dec 22, 2021

dieseldjango commented Feb 9, 2022

heath-freenome commented Feb 9, 2022 •

edited

Loading

nickgros commented Feb 10, 2022

epicfaace commented Feb 10, 2022 via email

Juansasa commented Feb 15, 2022

nickgros Feb 18, 2022

epicfaace Feb 18, 2022

nickgros Feb 18, 2022

epicfaace commented Feb 18, 2022

		resolvedSchemaLessConditional,
		retrieveSchema(conditionalSchema, rootSchema, formdata)

Support for if, else, then rules #2506

Support for if, else, then rules #2506

Conversation

Juansasa commented Aug 6, 2021 • edited Loading

Reasons for making this change

Checklist

Juansasa commented Aug 6, 2021

levyitay commented Aug 6, 2021

levyitay left a comment

Choose a reason for hiding this comment

nickgros commented Aug 11, 2021

Juansasa commented Aug 13, 2021 • edited Loading

nickgros commented Aug 13, 2021

epicfaace commented Aug 13, 2021

AlimovSV commented Aug 24, 2021

jmsaulnier commented Oct 29, 2021

heath-freenome commented Nov 17, 2021 • edited Loading

epicfaace Nov 18, 2021

Choose a reason for hiding this comment

epicfaace Nov 18, 2021

Choose a reason for hiding this comment

epicfaace commented Nov 18, 2021 • edited Loading

epicfaace commented Nov 18, 2021

nickgros commented Nov 18, 2021

jmsaulnier commented Dec 8, 2021

ksbrar commented Dec 22, 2021

dieseldjango commented Feb 9, 2022

heath-freenome commented Feb 9, 2022 • edited Loading

nickgros commented Feb 10, 2022

epicfaace commented Feb 10, 2022 via email

Juansasa commented Feb 15, 2022

nickgros Feb 18, 2022

Choose a reason for hiding this comment

epicfaace Feb 18, 2022

Choose a reason for hiding this comment

nickgros Feb 18, 2022

Choose a reason for hiding this comment

epicfaace commented Feb 18, 2022

Juansasa commented Aug 6, 2021 •

edited

Loading

Juansasa commented Aug 13, 2021 •

edited

Loading

heath-freenome commented Nov 17, 2021 •

edited

Loading

epicfaace commented Nov 18, 2021 •

edited

Loading

heath-freenome commented Feb 9, 2022 •

edited

Loading