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

Allow omitting cardinality when creating query key #485

Closed
wants to merge 1 commit into from

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

@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