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

feat: Generate titles from definition names #2127

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

FoxxMD
Copy link

@FoxxMD FoxxMD commented Dec 4, 2024

All root child definitions have a title added to their definition if the opt-in definitionTitles option is set

  • The title value is the same as the node name (which is, ideally, the name of the interface/type/alias from TS code).
  • Use definitionTitles: true programatically or from the command line with arg --definition-titles

Closes #1486

FoxxMD added a commit to FoxxMD/multi-scrobbler that referenced this pull request Dec 4, 2024
* Patch generator to generate definition titles until vega/ts-json-schema-generator#2127 is resolved
* Add launch config for debugging schema generation
* Disallow topRef generation so docusarus json viewer works
* Write to docsite static assets on generation
@@ -135,6 +135,9 @@ export class SchemaGenerator {
const name = child.getName();
if (!(name in definitions)) {
definitions[name] = this.typeFormatter.getDefinition(child.getType());
if (this.config?.definitionTitles) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if this is the best place. Something inside getDefinition should be better. This doesn't work for deep objects/definitions, either.

README.md Outdated Show resolved Hide resolved
@arthurfiorette
Copy link
Collaborator

@domoritz general idea seems fair, but I'm not sure if this is something we want to add (at least not just as an if statement in the SchemaGenerator.ts and not somewhere else)

@domoritz
Copy link
Member

domoritz commented Dec 8, 2024

What's the reason for having titles? I don't like having extra CLI actions when possible.

@FoxxMD
Copy link
Author

FoxxMD commented Dec 9, 2024

The reason for generating titles is that they are a valid property type for draft-07 json-schema and are used by some applications for displaying the names of definitions.

I use json-schema-plugin for Docusaurus to display my application's config to users in the documentation.

Without title properties on definitions the rendered schema is not user friendly.

no_title

With title properties on definitions the rendered objects have user-friendly names and the layout of the schema is simplified as well.

with_title

This is just the relevant example to me but I'm sure I'm not the only one who would benefit from having titles, since its a valid piece of the schema to consume.


I would have also preferred to avoid adding CLI args by instead using custom formatting for a definition but I could not find a way to get access to the node's name (CircularReferenceTypeFormatter) in a way that did not require refactoring.

In the last stage of appendRootChildDefinitions when reducing children definitions the final, rendered object comes from an ObjectType child of the parent CircularReferenceType which does not provide the derived name to this function.

If anyone has guidance on how I could better achieve this using a custom formatter I'd be happy to adapt this PR to do that instead but I was struggling for a simple way to get this result.

@arthurfiorette
Copy link
Collaborator

Seems valid to me, @domoritz your thoughts?

@FoxxMD please fix the formatting in the readme file as my above review.

@domoritz
Copy link
Member

domoritz commented Dec 9, 2024

I think we should make titles the default but also

  1. Make sure we url encode keys (which iirc is required by the spec) and use the non url encoded name as the title
  2. Use the correct title as much as possible. So if there are duplicate names for example, the key may need a prefix to be unambiguous but the title may be ambiguous.

Does that seem reasonable?

@arthurfiorette
Copy link
Collaborator

Titles could be the default but should be available under a configuration option as well, otherwise this release would invalidate all previous generated schemas.

Maybe non-default until next major?

@domoritz
Copy link
Member

Adding titles isn't a breaking change imo but should be a minor version bump. But I don't feel strongly about it.

@arthurfiorette
Copy link
Collaborator

It might not be a breaking change like builds failing, but this will increase the final JSON output by a considerable percentage since names will now be duplicated. Also, any kind of cache keys (hashed ones) will hash into different values even for the same input since a new field was added for every single schema present.

All root child definitions have a 'title' added to their definition if the opt-in definitionTitles option is set
@FoxxMD
Copy link
Author

FoxxMD commented Dec 10, 2024

Any thoughts regarding refactoring so this could be a custom formatter instead? I'll admit I don't fully grok the whole schema generation lifecycle but from what I observed while debugging:

Changing getDefinition signature in formatter

Adding a second name arg would solve this issue, with all applicable formatters returning their ObjectType child with the name passed. However, this doesn't support any users currently using a custom formatter (and potentially breaks them unless second arg is optional).

BaseType getName ?

This could be used but I don't fully understand why some types get a guid and some get the name of actual data structure used in TS. I might not be understanding this correctly though. If there was a way to set the data structure's name here, alongside the guid, then it could be used.

@domoritz
Copy link
Member

Any thoughts regarding refactoring so this could be a custom formatter instead?

Yeah, I think this should/could be implemented as a formatter similar to the annotation formatter. We can already get the title via a comment so we should also make sure that we are letting people override the title with a comment.

@FoxxMD
Copy link
Author

FoxxMD commented Dec 11, 2024

We can already get the title via a comment

can you elaborate on this a bit more? Where, in code?

@domoritz
Copy link
Member

I don't have the exact syntax handy but with @title or @tjs-title you can set properties that will appear in the schema.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support title
3 participants