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.

Sign up for GitHub

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

Jump to bottom

feat: improve type-safety (like the queryOptions type-helper) #1672

Merged
merged 4 commits into from
Oct 28, 2024
Merged

feat: improve type-safety (like the queryOptions type-helper) #1672

merged 4 commits into from
Oct 28, 2024

Conversation

AmitBar2001
Copy link
Contributor

@AmitBar2001 AmitBar2001 commented Oct 24, 2024 •
edited by melloware
Loading

Status

READY

Description

Fix #1671

following the implementation described here:
https://tkdodo.eu/blog/the-query-options-api#datatag

note: this is the first PR I've opened, I would very much appreciate your feedback.

Related PRs

List related PRs against other branches:

branch PR
other_pr_production link
other_pr_master link

Todos

  • Tests
  • Documentation
  • Changelog Entry (unreleased)

Steps to Test or Reproduce

Outline the steps to test or reproduce the PR here.

> git pull --prune
> git checkout <branch>
> grunt jasmine

AmitBar2001
AmitBar2001 commented Oct 24, 2024
View reviewed changes
packages/query/src/index.ts
Comment on lines -412 to 418
if (!params.length) {
if (!params.length || isSuspenseQuery(type)) {
if (options) {
return `${queryConfig} ...queryOptions`;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because suspense queries don't expect to be passed the 'enabled' option (https://tanstack.com/query/latest/docs/framework/react/reference/useSuspenseQuery), which is passed a few lines down from here after this return statement.
This failed a test, I do not know why this test hasn't failed before these changes but I think it somehow triggered this existing bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a note for the person reviewing this, you can resolve this after reading if you want.

@melloware melloware added this to the 7.2.1 milestone Oct 24, 2024
@melloware
Copy link
Collaborator

looks like something is messed up with the build?

@AmitBar2001
Copy link
Contributor Author

AmitBar2001 commented Oct 24, 2024 •
edited
Loading

looks like something is messed up with the build?

Yes, it fails in the install stage.
It seems to me unrelated to this PR's changes as package.json files were not modified.
image

I think I may have found the cause:
87e1188#diff-af75921c98100c36b0ba10db16be2bf8abd330024b5e63a9c219ffe4d9be5cceR17

The same dependencies that fail the install were bumped in this commit from 7.1.1 to 7.2.0, but it seems the yarn.lock file was not updated accordingly.
I noticed this section in the ci log output:
image

Maybe this is the reason this surfaced only now and not before, since this PR is from the fork I made.

Would you like me to add the update to the lockfile?
(generated from my local yarn install)

-- Of course i did not pay attention and clicked sync changes on my fork repo, without installing locally and committing locally, which would probably have caught this change earlier

@melloware
Copy link
Collaborator

Yes please it was probably from last release on Monday.

@melloware
Copy link
Collaborator

ok now run npm run format to format the files. Probably also not your fault

@melloware
Copy link
Collaborator

All good now! Will just let @soartec-lab or @anymaniax review

@soartec-lab
Copy link
Member

Thank you! I would like to see this PR, but I need a few days.

soartec-lab
soartec-lab approved these changes Oct 28, 2024
View reviewed changes
Copy link
Member

@soartec-lab soartec-lab left a comment

Choose a reason for hiding this comment

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

Thanks for nice update!

@soartec-lab soartec-lab merged commit b24ef85 into orval-labs:master Oct 28, 2024
2 checks passed
@soartec-lab soartec-lab added the enhancement New feature or request label Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soartec-lab soartec-lab soartec-lab approved these changes

@melloware melloware melloware approved these changes

@anymaniax anymaniax Awaiting requested review from anymaniax

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
7.3.0
Development

Successfully merging this pull request may close these issues.

Query: improve type-safety (like the queryOptions type-helper)
3 participants
@AmitBar2001 @melloware @soartec-lab