ngx-formly fix: merge schema properties with definitions $ref

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Remove some duplicated code
Corrects properties with the use of a definition

to have the same behavior as here https://rjsf-team.github.io/react-jsonschema-form/

Try with :

{
          definitions: {
            address: {
              properties: {
                address1: { type: 'string', title: 'Address 1' },
              },
            },
          },
          type: 'object',
          properties: {
            billing_address: {
              properties: {
                customAddress: { type: 'string', title: 'Custom address' },
              },
              $ref: '#/definitions/address',
            },
          },
        }

What is the current behavior? (You can also link to an open issue here)

Only add definitions properties

What is the new behaviour (if this is a feature change)?

Merge add definitions properties and parent one

Please check if the PR fulfills these requirements

[x] The commit messages follow our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
[x] A unit test has been written for this change.
[x] Running npm run build produced a successful build. (Unit testing can be done by running npm test;)
[x] My code has been linted. (npm run lint to do this. npm run build will fail if there are files not linted.)

Please provide a screenshot of this feature before and after your code changes, if applicable.

Other information:

Feb 25 '23 09:02 ThibaudAV

Deploy Preview for formly-dev ready!

Name	Link
Latest commit	2e8cdb7da899af0505e7154ae367f5453061b038
Latest deploy log	https://app.netlify.com/sites/formly-dev/deploys/63f9d0dff1d8eb00085d027f
Deploy Preview	https://deploy-preview-3604--formly-dev.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Feb 25 '23 09:02 netlify[bot]

Based on the Jason schema spec the current implementation is correct.

Definitions should only be mered at the annotation level not at the validation level.

In order to get what you'd like here you should be using all of instead of properties definition + ref

On Sat, Feb 25, 2023, 3:15 AM netlify[bot] @.***> wrote:

✅ Deploy Preview for formly-dev ready! Name Link 🔨 Latest commit 2e8cdb7 https://github.com/ngx-formly/ngx-formly/commit/2e8cdb7da899af0505e7154ae367f5453061b038 🔍 Latest deploy log https://app.netlify.com/sites/formly-dev/deploys/63f9d0dff1d8eb00085d027f 😎 Deploy Preview https://deploy-preview-3604--formly-dev.netlify.app 📱 Preview on mobile Toggle QR Code...

[image: QR Code] https://camo.githubusercontent.com/f303f8b204c4a1ebb7e8dac755e77043ec0694d4639c8a4926074398032086bb/68747470733a2f2f6170702e6e65746c6966792e636f6d2f71722d636f64652f65794a30655841694f694a4b563151694c434a68624763694f694a49557a49314e694a392e65794a31636d77694f694a6f64485277637a6f764c32526c6347787665533177636d5632615756334c544d324d4451744c575a76636d31736553316b5a585975626d563062476c6d655335686348416966512e4d3574765532686f467038645442535039573977737343475a78663872623873534b3048756f33446e4155

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings https://app.netlify.com/sites/formly-dev/settings/deploys#deploy-notifications.

— Reply to this email directly, view it on GitHub https://github.com/ngx-formly/ngx-formly/pull/3604#issuecomment-1445037795, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADB4XNMKQR4TPPABVHVFOD3WZHERPANCNFSM6AAAAAAVHX7KME . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Feb 25 '23 22:02 kenisteward

For reference https://json-schema.org/understanding-json-schema/structuring.html#ref

On Sat, Feb 25, 2023, 4:36 PM Kenneth Steward @.***> wrote:

Based on the Jason schema spec the current implementation is correct.

Definitions should only be mered at the annotation level not at the validation level.

In order to get what you'd like here you should be using all of instead of properties definition + ref

On Sat, Feb 25, 2023, 3:15 AM netlify[bot] @.***> wrote:

✅ Deploy Preview for formly-dev ready! Name Link 🔨 Latest commit 2e8cdb7 https://github.com/ngx-formly/ngx-formly/commit/2e8cdb7da899af0505e7154ae367f5453061b038 🔍 Latest deploy log https://app.netlify.com/sites/formly-dev/deploys/63f9d0dff1d8eb00085d027f 😎 Deploy Preview https://deploy-preview-3604--formly-dev.netlify.app 📱 Preview on mobile Toggle QR Code...

[image: QR Code] https://camo.githubusercontent.com/f303f8b204c4a1ebb7e8dac755e77043ec0694d4639c8a4926074398032086bb/68747470733a2f2f6170702e6e65746c6966792e636f6d2f71722d636f64652f65794a30655841694f694a4b563151694c434a68624763694f694a49557a49314e694a392e65794a31636d77694f694a6f64485277637a6f764c32526c6347787665533177636d5632615756334c544d324d4451744c575a76636d31736553316b5a585975626d563062476c6d655335686348416966512e4d3574765532686f467038645442535039573977737343475a78663872623873534b3048756f33446e4155

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings https://app.netlify.com/sites/formly-dev/settings/deploys#deploy-notifications.

— Reply to this email directly, view it on GitHub https://github.com/ngx-formly/ngx-formly/pull/3604#issuecomment-1445037795, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADB4XNMKQR4TPPABVHVFOD3WZHERPANCNFSM6AAAAAAVHX7KME . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Feb 25 '23 22:02 kenisteward

It's important for us not to introduce this especially for teams that share schemas with validators on the backend as those validators would not be interested this way.

On Sat, Feb 25, 2023, 4:37 PM Kenneth Steward @.***> wrote:

For reference https://json-schema.org/understanding-json-schema/structuring.html#ref

On Sat, Feb 25, 2023, 4:36 PM Kenneth Steward @.***> wrote:

Based on the Jason schema spec the current implementation is correct.

Definitions should only be mered at the annotation level not at the validation level.

In order to get what you'd like here you should be using all of instead of properties definition + ref

On Sat, Feb 25, 2023, 3:15 AM netlify[bot] @.***> wrote:

✅ Deploy Preview for formly-dev ready! Name Link 🔨 Latest commit 2e8cdb7 https://github.com/ngx-formly/ngx-formly/commit/2e8cdb7da899af0505e7154ae367f5453061b038 🔍 Latest deploy log https://app.netlify.com/sites/formly-dev/deploys/63f9d0dff1d8eb00085d027f 😎 Deploy Preview https://deploy-preview-3604--formly-dev.netlify.app 📱 Preview on mobile Toggle QR Code...

[image: QR Code] https://camo.githubusercontent.com/f303f8b204c4a1ebb7e8dac755e77043ec0694d4639c8a4926074398032086bb/68747470733a2f2f6170702e6e65746c6966792e636f6d2f71722d636f64652f65794a30655841694f694a4b563151694c434a68624763694f694a49557a49314e694a392e65794a31636d77694f694a6f64485277637a6f764c32526c6347787665533177636d5632615756334c544d324d4451744c575a76636d31736553316b5a585975626d563062476c6d655335686348416966512e4d3574765532686f467038645442535039573977737343475a78663872623873534b3048756f33446e4155

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings https://app.netlify.com/sites/formly-dev/settings/deploys#deploy-notifications.

— Reply to this email directly, view it on GitHub https://github.com/ngx-formly/ngx-formly/pull/3604#issuecomment-1445037795, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADB4XNMKQR4TPPABVHVFOD3WZHERPANCNFSM6AAAAAAVHX7KME . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Feb 25 '23 22:02 kenisteward

I wanted to test with an allOf but it gives me a result that is not what I expected 😁

Json Schem

{
"type": "object",
"definitions": {
  "userType": {
    "oneOf": [
      {
        "title": "Human",
        "properties": {
          "pseudo": {
            "type": "string",
            "title": "Pseudo"
          }
        },
        "required": [
          "pseudo"
        ]
      },
      {
        "title": "Robot",
        "properties": {
          "serialNumber": {
            "type": "string",
            "title": "Serial number"
          }
        },
        "required": [
          "serialNumber"
        ]
      }
    ]
  },
  "authMethod": {
    "oneOf": [
      {
        "title": "With token",
        "properties": {
          "token": {
            "type": "string",
            "title": "Token"
          }
        },
        "required": [
          "token"
        ]
      },
      {
        "title": "With username and password",
        "properties": {
          "username": {
            "type": "string",
            "title": "Username"
          },
          "password": {
            "type": "string",
            "title": "Password"
          }
        },
        "required": [
          "username"
        ]
      }
    ]
  }
},
"properties": {
  "allOfWithProperties": {
    "title": "AllOf with properties",
    "type": "object",
    "allOf": [
      {
        "properties": {
          "first": {
            "title": "First",
            "type": "string"
          }
        }
      },
      {
        "$ref": "#/definitions/authMethod"
      },
      {
        "properties": {
          "second": {
            "title": "Second",
            "type": "string"
          }
        }
      },
      {
        "$ref": "#/definitions/userType"
      }
    ]
  }
}
}

I would have expected to have all fiels and multishema in the allOf order. But apparently the AllOf with OneOf not worked as i expect. Only one "OneOf" are displayed and not in the right order.

Feb 26 '23 11:02 ThibaudAV

I close it. I hope to find other solutions for my use case. If you have suggestions, do not hesitate 🙂

Feb 28 '23 21:02 ThibaudAV

Sorry just saw the updates to this. Could you setup a stackblitz and start an issue so I can see what output you want vs what output yoy are getting ?

On Tue, Feb 28, 2023, 3:21 PM ThibaudAV @.***> wrote:

Closed #3604 https://github.com/ngx-formly/ngx-formly/pull/3604.

— Reply to this email directly, view it on GitHub https://github.com/ngx-formly/ngx-formly/pull/3604#event-8631589485, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADB4XNMYYNSFUYPWS5Q6GKTWZZT63ANCNFSM6AAAAAAVHX7KME . You are receiving this because you commented.Message ID: @.***>

Mar 01 '23 15:03 kenisteward

Or course : https://stackblitz.com/edit/angular-jumm2x?file=src%2Fapp%2Fapp.component.ts,src%2Fapp%2Fapp.component.html

I would expect to have the output fields in the order :

First field
authMethod ref fields
Second field
userType ref fields

Thank you for your help 🙏

Mar 02 '23 17:03 ThibaudAV

Ahh yes! It looks like we might currently have a limitation that we can't merge disparate oneOf's that are across the same object.

If they are on disparate keys it looks like it works great https://stackblitz.com/edit/angular-jumm2x-jevk23?file=src%2Fapp%2Fapp.component.ts,src%2Fapp%2Fapp.component.html

If you don't mind, could you make an issue about this that we can track? It's very possible that we may need to spend some investigation time on the PR you made here to do the merging this way to alleviate this problem but there may be some other solution as well. We also could add an option to the formly field generate that is something like "mergeReferences" that would allow this to work for the normal use case

what do you think? @aitboudad?

On Thu, Mar 2, 2023 at 11:12 AM ThibaudAV @.***> wrote:

Or course : https://stackblitz.com/edit/angular-jumm2x?file=src%2Fapp%2Fapp.component.ts,src%2Fapp%2Fapp.component.html

I would expect to have the output fields in the order :

First field

authMethod ref fields

Second field

userType ref fields

Thank you for your help 🙏

— Reply to this email directly, view it on GitHub https://github.com/ngx-formly/ngx-formly/pull/3604#issuecomment-1452226110, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADB4XNNFPTBVTX434O7RKJDW2DIIXANCNFSM6AAAAAAVHX7KME . You are receiving this because you commented.Message ID: @.***>

Mar 02 '23 20:03 kenisteward

I'll revised this once time allows :pray:, have you tried adjusting the order through the map option https://github.com/ngx-formly/ngx-formly/issues/1982#issuecomment-566048527

Mar 03 '23 11:03 aitboudad

@ThibaudAV what @aitboudad does fix the ordering issue! I forgot this was added in order to define what order the fields which fixed that.

The other issue of logic for merging oneOf's seems like it's a bigger discussion ticket though.

In the context of validations it makes sense because jsonschema is just a validation language that parses them disparately as separate rules. When it comes to generating fields though we have to decide if we merge them what exactly happens. Right now it looks like first one wins ( or last one I can't remember haha )

Mar 03 '23 16:03 kenisteward

@kenisteward

If they are on disparate keys it looks like it works great https://stackblitz.com/edit/angular-jumm2x-jevk23?file=src%2Fapp%2Fapp.component.ts,src%2Fapp%2Fapp.component.html

This changes the structure of the object and I can't change that

No the order does not fix my problem :/

And yes for OneOf merger. Maybe there is something to change on this side. maybe add them in a respective sub form. or something like that

Mar 06 '23 18:03 ThibaudAV

ngx-formly ngx-formly copied to clipboard

fix: merge schema properties with definitions $ref

✅ Deploy Preview for formly-dev ready!

Use your smartphone camera to open QR code link.

Use your smartphone camera to open QR code link.

Use your smartphone camera to open QR code link.

ngx-formly
ngx-formly copied to clipboard

Deploy Preview for formly-dev ready!