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

Remove optional flag in case of default in schema. #558

Open
kityan opened this issue Nov 2, 2023 · 8 comments
Open

Remove optional flag in case of default in schema. #558

kityan opened this issue Nov 2, 2023 · 8 comments

Comments

@kityan
Copy link

kityan commented Nov 2, 2023

Correct me if I'm wrong, but nowadays the converter follows the strict rule: make prop optional if it's not in required list.
But JSON Schema allows using key default. Of course, existence of the prop in real object depends on realization (e.g.: schema-based validator).
I use ajv in my json-rpc server pipeline to validate arguments with useDefaults:true: it injects default values (defined in schema) in case of absent props.
That's why I'd like to have optional flag removed from such types.
If you feel this is good idea, I can try to fork and make a pull request soon. I think this behavior should be optional, e.g.: `removeOptionalIfDefaultExists"

@kityan
Copy link
Author

kityan commented Nov 9, 2023

May be it's not good idea at all. From a server side view it's OK to remove optional flag because default value is injected at validation stage.
But if we use the same type definition for a client — the optional flag should remain.

@kityan
Copy link
Author

kityan commented Nov 11, 2023

Anyway, the option I've suggested earlier will be useful. For example, in our development process we use two separate ts-files (with defaults injected - for server and without - for clients) converted from one json schema.
In this case client knows wich props are optional, and server thinks nothing optional because of injected defaults and we have less code in RPC methods.

@leppaott
Copy link

leppaott commented Dec 4, 2023

Just add them to the required lists? In the past I had experience that required was checked before default so didn't work necessarily but now I'm setting required and having default and it works even when not given (i.e. defaulted passes required check). The issue with the proposal indeed is what if useDefaults:true is not given, required solves that.

@GabenGar
Copy link

GabenGar commented Jan 1, 2024

"default" is an annotation keyword like "title" or "description". it doesn't make sense in the context of typescript types, as there is no such thing as "default value" for a type. On AJV side it becomes a footgun, since its value is not validated, so its type also can drift away from the schema type. Also some values can't be serialized or not known at validation time, such as home directory, so the type being optional or not varies on case-by-case basis on top.
Your mistake is using json schema as data modeling DSL instead of JSON payload validation DSL. JSON, and therefore JSON Schema, will never be able to map neatly into types of other languages.

@kityan
Copy link
Author

kityan commented Jan 1, 2024

@leppaott

Just add them to the required lists? In the past I had experience that required was checked before default so didn't work necessarily but now I'm setting required and having default and it works even when not given (i.e. defaulted passes required check). The issue with the proposal indeed is what if useDefaults:true is not given, required solves that.

Not really. Look — I use json schema files for:

  • validate request on backend
  • generate ts for backend
  • generate ts for frontend
  • generate docs (including defaults values!!!)

Also:

  1. I don't want to check at backend if optional param undefined and set to default in code. I want to do it by schema.
  2. I don't want to see at backend that the param is optional and use args.param! because I known — it is either sent by frontend or set to default by validator
  3. I want to see param as optional at frontend.

You see?
So my solution (when I generate ts for back and front) — to treat default: ... differently. For frontend I don't take it into account, only analyze "required:[...]" to see — optional or not. But for backend, I analyze default: .... to see if param is never undefined.

@kityan
Copy link
Author

kityan commented Jan 1, 2024

@GabenGar

Your mistake is using json schema as data modeling DSL instead of JSON payload validation DSL.

Don't judge before you try. So, your advice — don't use defaults in json schema, put this into code? Produce more imperative code instead of declarative? Funny.

@kityan
Copy link
Author

kityan commented Jan 1, 2024

@GabenGar

it doesn't make sense in the context of typescript types, as there is no such thing as "default value" for a type.

Of course. But it does make sense in context: would be param undefined or not (event if it's not required). So it's related to treating param is optional or not regardless it's not in required:[]

@FractalHQ
Copy link

tsconfig.json is often the only file in a project of mine where default values aren't displayed by intellisense. I always have to click a link to the typescript website to see the default value of each compiler option, and it's bummed me out for years!

For this narrow, selfish reason; I would be happiest with @default (or tsdoc @defaultValue) tags 😅

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

No branches or pull requests

4 participants