Skip to content
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

Separate JSON schema for each component #1283

Closed
wants to merge 14 commits into from

Conversation

nkylstad
Copy link
Member

@nkylstad nkylstad commented Jun 26, 2023

Description

  • Split the large layout.schema.v1.json into separate schemas for each component, which are referenced from a new layout.schema.v2.json
  • Definitions that are common/re-used in many components are added to the common-defs.schema.json file, which only contains definitions.
  • Basically all the components have the properties showAsSummary, hidden, grid and pageBreak, as I'm not sure if there are any components that do not support these. Feel free to comment/suggest changes here.
  • I have tried to keep f.ex. dataModelBindings and textResourceBindings as component specific as possible, but it is possible I have missed/forgotten something - again feel free to update/comment.
  • I added a script for generating the base for the component schema and adding reference to this to the new layout.schema.v2.json
  • One issue I encountered was the "either/or" nature of the options/optionsId properties for selection components. I'm sure it's possible to create a conditional schema where:
    • it's not allowed with both properties together
    • options is required when optionsId is not present
    • optionsId is required when options is not present
      However I was not able to get this working. As it's not a feature of the current schema either I left it out for now.

I'm also slightly unsure of whether the relative paths to the linked schemas work in the case of schemas in a different folder. It works when I test it locally, but I imagine it might be different once this is published to CDN - we might have to use complete URLs for schemas that do not live in the same folder 🤔

I will also need some help updating any pipelines/actions that publish schemas to CDN if there is a need for that.

Related Issue(s)

Verification/QA

  • Manual functionality testing
    • I have tested these changes manually (i have tested creating a layout and seeing that it validates as expected)
    • Creator of the original issue (or service owner) has been contacted for manual testing (or will be contacted when released in alpha)
    • No testing done/necessary
  • Automated tests
    • Unit test(s) have been added/updated
    • Cypress E2E test(s) have been added/updated
    • No automatic tests are needed here (no functional changes/additions)
    • I want someone to help me make some tests
  • UU/WCAG (follow these guidelines until we have our own)
    • I have tested with a screen reader/keyboard navigation/automated wcag validator
    • No testing done/necessary (no DOM/visual changes)
    • I want someone to help me perform accessibility testing
  • User documentation @ altinn-studio-docs
    • Has been added/updated
    • No functionality has been changed/added, so no documentation is needed
    • I will do that later/have created an issue
  • Changes/additions to component properties
    • Changes are reflected in both src/layout/layout.d.ts and layout.schema.v1.json, and these are all backwards-compatible
    • No changes made
  • Support in Altinn Studio
    • Issue(s) created for support in Studio
    • This change/feature does not require any changes to Altinn Studio
  • Sprint board
    • The original issue (or this PR itself) has been added to the Team Apps project and to the current sprint board
    • I don't have permissions to do that, please help me out
  • Labels
    • I have added a kind/* label to this PR for proper release notes grouping
    • I don't have permissions to add labels, please help me out

@nkylstad nkylstad added the kind/other Pull requests containing chores/repo structure/other changes label Jun 26, 2023
@olemartinorg
Copy link
Contributor

I'm also slightly unsure of whether the relative paths to the linked schemas work in the case of schemas in a different folder. It works when I test it locally, but I imagine it might be different once this is published to CDN - we might have to use complete URLs for schemas that do not live in the same folder 🤔

Should be easy enough to test locally with http-server. I'll look into it!

I will also need some help updating any pipelines/actions that publish schemas to CDN if there is a need for that.

Should not be needed - the whole schemas folder is copied over.

I'll add a list of TODOs I think we should look into after this:

  • Test references to external schemas inside a folder to make sure it works when deploying to CDN
  • Run tests to check existing apps, errors and differences between this schema and the old schema. (See src/utils/layout/schema.test.ts)
  • Move this over to v1 if it's not a breaking change (if it is, work out a strategy for migrating apps to v2)
  • Add automatic formatting with prettier? We disabled this for the older schema because we didn't want to reformat that file when adding/changing a few lines. With a clean slate, we can clean it up as we go.
  • Auto-genereate layout schema based on TypeScript interfaces #832 🙏

@bjosttveit
Copy link
Member

I have tried to keep f.ex. dataModelBindings and textResourceBindings as component specific as possible, but it is possible I have missed/forgotten something - again feel free to update/comment.

Quickly looked over a few components and there are likely many missing textResourceBinding properties. Not sure if this is up-to-date anymore but: https://altinndevops.slack.com/archives/C045EB3JA9X/p1675931223052249?thread_ts=1675871625.344949&cid=C045EB3JA9X

@olemartinorg
Copy link
Contributor

@bjosttveit I think we should merge this as-is, to let @nkylstad carry on - and in a separate job we should look over our text resource bindings for each component and properly type them in both TypeScript and in this schema. Would be a nice way to start off #832, maybe.

@bjosttveit
Copy link
Member

@bjosttveit I think we should merge this as-is, to let @nkylstad carry on - and in a separate job we should look over our text resource bindings for each component and properly type them in both TypeScript and in this schema. Would be a nice way to start off #832, maybe.

Then maybe switch additionalProperties to true in textResourceBindings for now? Or people will get a ton of warnings and it would be confusing. Some components like Link require the target to be set in textResourceBindings.

@nkylstad
Copy link
Member Author

nkylstad commented Jun 26, 2023

@bjosttveit I think we should merge this as-is, to let @nkylstad carry on - and in a separate job we should look over our text resource bindings for each component and properly type them in both TypeScript and in this schema. Would be a nice way to start off #832, maybe.

Then maybe switch additionalProperties to true in textResourceBindings for now? Or people will get a ton of warnings and it would be confusing. Some components like Link require the target to be set in textResourceBindings.

Sure, I have set additionalProperties: true for all textResourceBindings now. Then you can restrict it back for the relevant components when you're sure which keys are the correct ones 👍
I also updated with some of the keys mentioned in the slack thread, but don't think I got them all, and they should probably be documented better.

@FinnurO
Copy link

FinnurO commented Jun 27, 2023

Har man glemt denne settingen som kom nylig på Radiogroup? https://github.com/orgs/Altinn/projects/39/views/2?pane=issue&itemId=30673603

@nkylstad
Copy link
Member Author

Har man glemt denne settingen som kom nylig på Radiogroup? https://github.com/orgs/Altinn/projects/39/views/2?pane=issue&itemId=30673603

Ja! Glemt er kanskje å ta litt hardt i gitt at PR'en for den radio-endringen ble merget i går 😅 Men tar det med!

@FinnurO
Copy link

FinnurO commented Jun 28, 2023

Vi har vel også en "PrintButton"? Brukes i anonym oppstartsveileder.

@olemartinorg olemartinorg marked this pull request as draft June 28, 2023 13:43
@olemartinorg
Copy link
Contributor

Marking this as draft because I'm working on adding support for the 3 missing components here. After that is done, I want to compare the results of validating against this file and the old one on existing apps to make sure we know of every case where this might break an existing configuration.

@olemartinorg olemartinorg self-assigned this Jun 28, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@thestokkan
Copy link

Hi! I've created Hugo shortcode scripts to automatically list each component's properties in the documentation and have come across a few issues:

@olemartinorg
Copy link
Contributor

@thestokkan Oh, it's possible we're working on reaching the same goals, then.. 🙈 I'm currently working on #832 now, and I'm creating a code generator that will generate both these JsonSchema files (making this PR unnecessary) and our internal TypeScript types. I'm thinking it should also be possible for that code generator to generate some documentation in the form of markdown files as well. Instead of manually making sure every component property of all current components were correct in this PR, I opted to start implementing #832 instead, so that's why this PR has been dormant for a while.

@thestokkan
Copy link

thestokkan commented Jul 31, 2023

@olemartinorg Aha 😅 I was aware of someone working on autogenerating the schema files, but not documentation (I asked @nkylstad before the holiday if anyone was working on autogenerating documentation from the schema files and she said no 🤷🏻‍♀️). I just merged a PR with the shortcode files (Altinn/altinn-studio-docs#1022) and a separate PR with drafts for template files and a component example using the shortcode (Altinn/altinn-studio-docs#1021) (I'm going to ask for feedback on the template on Slack). Why don't you have a look and then we figure out the best solution? I'm a Hugo newbie so you can most likely create something more robust. 😉

Edit: I saw the below comment on your PR just now. This is a bit strange, as I believe @TheTechArch knew at that point what I was working on. Maybe there's been a misunderstanding somewhere.

An idea from @TheTechArch: We could also generate markdown for docs.altinn.studio, so that each of our components get a page detailing every property that could/should be set.

@olemartinorg
Copy link
Contributor

@thestokkan Very interesting! 🤔🤩 I still have some work left on #832, but I would love to work together with you and @nkylstad to make something work. I believe JsonSchema is just a bit too complicated to parse for this purpose, so I think we can generate a new format that can be used to automatically create markdown for component properties docs, and also for generating a configuration Altinn Studio can use to get some hints at what is needed for the component configuration. If that spins out into 4 different formats (our internal TypeScript types, the JsonSchema used for validation + vscode hinting, a simplified structure for documentation, and another format for Studio) that should be easily possible with the code generator I'm making.

So, I suggest we work together on this one! 🙏 I'll continue working on #832, but I'll make sure to reach out when that's more stable.

@thestokkan
Copy link

@olemartinorg Sounds great!
And I can attest to json schema being difficult to parse! 😆 I have managed to do it, but the presentation of the components is probably not the best.

Question is, should I keep the shortcode for now until we have something else? I've used the json schema links from this PR waiting for it to be merged. Not sure if that is ok as a temporary solution?

@olemartinorg
Copy link
Contributor

@thestokkan I suspect the new JsonSchema will look radically different, so even though parsing this JsonSchema was possible, parsing every possible valid varation in JsonSchema is close to impossible - so using the JsonSchema itself for parsing to docs makes for a solution that will easily break, and/or limit our creativity when extending the schema.. The same thing goes for using the schema as a basis for an editor (i.e. Studio).

So yeah, keep it as-is, if you have other things to work on, and we'll touch base to figure out a longer-lasting solution. 🙌 Btw, I looked briefly at the hugo shortcode, and it did not look like newbie code to me! 🤷 Nifty solution! 🤩

@olemartinorg
Copy link
Contributor

The new (generated) JsonSchema is on its way in #1406. Closing this PR, and we'll figure out a solution to the docs and Studio side of things discussed above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/other Pull requests containing chores/repo structure/other changes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants