-
Notifications
You must be signed in to change notification settings - Fork 556
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: merge schema properties with definitions $ref #3604
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for formly-dev ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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
> <2e8cdb7>
> 🔍 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
> <#3604 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ADB4XNMKQR4TPPABVHVFOD3WZHERPANCNFSM6AAAAAAVHX7KME>
> .
> You are receiving this because you are subscribed to this thread.Message
> ID: ***@***.***>
>
|
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
>> <2e8cdb7>
>> 🔍 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
>> <#3604 (comment)>,
>> or unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/ADB4XNMKQR4TPPABVHVFOD3WZHERPANCNFSM6AAAAAAVHX7KME>
>> .
>> You are receiving this because you are subscribed to this thread.Message
>> ID: ***@***.***>
>>
>
|
I wanted to test with an allOf but it gives me a result that is not what I expected 😁 Json Schem
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. |
I close it. |
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 <#3604>.
—
Reply to this email directly, view it on GitHub
<#3604 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADB4XNMYYNSFUYPWS5Q6GKTWZZT63ANCNFSM6AAAAAAVHX7KME>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
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 :
Thank you for your help 🙏 |
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
<#3604 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADB4XNNFPTBVTX434O7RKJDW2DIIXANCNFSM6AAAAAAVHX7KME>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
I'll revised this once time allows 🙏, have you tried adjusting the order through the map option #1982 (comment) |
@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 ) |
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 |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
to have the same behavior as here https://rjsf-team.github.io/react-jsonschema-form/
Try with :
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
npm run build
produced a successful build. (Unit testing can be done by runningnpm test
;)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: