Skip to content

Conversation

@anuraaga
Copy link
Contributor

cardinality allows setting undefined but requires being set. It seems easier to allow omission for users that don't differentiate between finite and infinite queries in their code

Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
@paul-sachs
Copy link
Collaborator

paul-sachs commented Dec 5, 2024

Hi @anuraaga. Appreciate the time you put into this PR but we had decided to keep this parameter explicit intentionally. The idea was that since the cardinality changes how the information is shaped and stored for queries, the developer should be explicit. This makes it a little more cumbersome for query client operations but that's why I'm pitching the custom query client to make that a little more digestible.

@anuraaga
Copy link
Contributor Author

anuraaga commented Dec 5, 2024

Ok @paul-sachs. I understand the motivation somewhat but do think that passing undefined explicitly is uncommon and doesn't look like idiomatic typescript IMO. If wanting to be explicit probably undefined should have been left out, but that would be a breaking change so not worth it now I guess.

Hopefully the custom client will clean things up.

@anuraaga anuraaga closed this Dec 5, 2024
@paul-sachs
Copy link
Collaborator

Yeah that is a fair statement. This API has grown to be pretty complicated due to increased complexity in the keys themselves. We should have spent time making sure all parameters were as explicit as possible, with readable defaults. I do believe having an opinionated query client will help to clear up the more complicated internal bits so we appreciate your patience.

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.

2 participants