feat: make all params optional #858

[Maxim-Mazurok]

Status

READY

Description

Makes all query params optional to resolve #858

Todos

• Tests
• Documentation - not needed IMO
• Changelog Entry (unreleased) - not sure what to do about it
• Resolve conflicts/cleanup
• Make it a configurable non-default option so not to break other people

Steps to Test or Reproduce

See tests https://github.com/Maxim-Mazurok/orval/blob/858/samples/vue-query/src/tests/all-params-optional.test-d.ts

[vercel]

Someone is attempting to deploy a commit to a Personal Account owned by @anymaniax on Vercel.

@anymaniax first needs to authorize it.

[anymaniax]

Hello @Maxim-Mazurok, could it cause breaking chance for others?

[WickyNilliams]

commented on the issue, before seeing the PR. so re-posting here to keep closer to the code:

why make them optional? shouldn't you follow whatever the schema says? if you want them optional, you can wrap the generated composable in your own which handles this, no?

[Maxim-Mazurok]

Hello @Maxim-Mazurok, could it cause breaking change for others?

Yes, I will make this configurable

why make them optional?

Because otherwise, Orval has a feature that doesn't make sense.
See https://github.com/anymaniax/orval/blob/ed1261af7ff773ca1338302e313ac8f02c3da00c/samples/vue-query/src/api/endpoints/petstoreFromFileSpecWithTransformer.ts#L319
The query is only enabled when all parameters are not falsy.
It is quite handy, especially when you chain multiple queries. Then you can do something like:

const getPetsQuery = useGetPetsQuery();
const firstPetId = computed(() => getPetsQuery.data.value?.data?.[0]);
const petInfoQuery = useGetPetInfoQuery({ petId: firstPetId });

This will magically fetch pets, and then as soon as they are fetched - it'll fetch the first pet info.

shouldn't you follow whatever the schema says?

Well, that's a good point, I guess you might end up with invalid queries this way.
For example, you could have

const firstPetId = computed(() => undefined);
const petInfoQuery = useGetPetInfoQuery({ petId: firstPetId });

In that example firstPetId will always be undefined and your query will never be enabled.
I guess that's a downside.

However, there's another interesting thing: if you make petId not required in schema - you will end up with petId: MaybeRef<string> | undefined type. This kinda breaks your reactivity, you can either pass a reactive string, or a non-reactive undefined. You can't pass reactive undefined that will become a string. And this kinda kills the beauty of Vue Query for me.

if you want them optional, you can wrap the generated composable in your own which handles this, no?

Yeah, I guess that could be an option. However, this kinda defeats the purpose of using Orval... I don't want to manually wrap generated composables, I want to reduce manual to a minimum by generating composables that work for me.

[WickyNilliams]

Because otherwise, Orval has a feature that doesn't make sense.

this is actually my opinion on the matter - i don't think the feature does makes sense! i personally think that adding enable behavior is a mistake, and loosening the interpretation of the schema to support the feature compounds that mistake. a tool like orval should strictly adhere to the schema, and should bake as close to zero assumptions into the generated code as possible.

for generated composables, i don't want them to try to be smart about enabling the query or not. i'd rather a TS error if i have passed the incorrect type. it would be a pain having to dig into generated files to work out why a query is silently not running. as a developer if i want to disable a query until some conditions are met, it's much easier for me to code this and get it right for my use case, than for orval to take a guess at it, and potentially get it very wrong.

things get even more confusing if e.g. there are boolean props. i can legitimately pass false and suddenly my query is disabled. likewise for falsiness. orval cannot understand the context or meaning of values, so it should not attempt to

of course i can override orval's own by passing enable myself, but then we're back at square one, and all of the work that orval has done gets thrown away.

This will magically fetch pets, and then as soon as they are fetched - it'll fetch the first pet info.

i don't see why this is less preferable?

const getPetsQuery = useGetPetsQuery();
const firstPetId = computed(() => getPetsQuery.data.value?.data?.[0]);
const enable = computed(() => firstPetId != null)

const petInfoQuery = useGetPetInfoQuery({ petId: firstPetId, enable });

sure it's one more line of code, but it's clear and self explanatory and does not rely on any magic 🧙

you will end up with petId: MaybeRef<string> | undefined

good point. i think this is a bug in orval and the type should be MaybeRef<string | undefined> (and the property should be optional also)

Yeah, I guess that could be an option. However, this kinda defeats the purpose of using Orval

whenever i've written query composables by hand, most of them do not use the enable functionality. it's been the exception not the rule. so the generated composables would be usable as-is most of the time. i don't want a tool like orval to eliminate writing any composables, just 95% of them. writing a wrapper for the 5% case is a fine trade off for greater correctness in general.

[Maxim-Mazurok]

I think most of the arguments are fair, I found it a bit unexpected that some queries were disabled due to falsy values.

However, we've been using this approach for a while now, including this PR as a patch in our project. So I would like to keep it for now.

Perhaps you could create a PR that adds a non-default configurable option to disable the "only enable when values are truthy" feature? This way it won't break existing users. And I would be interested in giving that PR a try.

---

Also, I just remembered that this feature and this PR are only about route/url params. I'd say it's quite unlikely you'll have boolean type in the route, like /api/pet/false/details or something like that. You can still pass falsy values without issues as query params like ?limit=false, etc.

---

My plan for this PR:

• Resolve conflicts/cleanup
• Make it a configurable non-default option so not to break other people
• Add tests

Would you guys agree to this plan, or would you still insist on removing this feature altogether?
@anymaniax @WickyNilliams

[melloware]

Looks like there are conflicts with this PR.

[Maxim-Mazurok]

Looks like there are conflicts with this PR.

Hello, yes, I'm back from the holidays and going to continue work on my PRs for Orval this or next week, will also connect with @anymaniax on discord, cheers :)

[Maxim-Mazurok]

This is the core change directly related to the issue

[Maxim-Mazurok]

@WickyNilliams this is ready for review now if you'd like to have a look

[Maxim-Mazurok]

It looks like @WickyNilliams is more involved in other projects, so I'll merge it for now to avoid conflicts, but still open to feedback, cheers!