Skip to content

feat(vue): refactor composable functions #3619

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

Merged
merged 24 commits into from
Jul 26, 2024

Conversation

yurks
Copy link
Contributor

@yurks yurks commented Jun 18, 2024

Resolves #3507
Resolves #3633

Summary

This includes #3612, but fixed reactivity loosing here.

In addition, a little refactoring for use* composable functions for reducing code duplication and adding more test cases.

Set of changes

  • Remove wrapping args in reactive in useQuery.ts and useSubscription.ts
  • Add createRequestWithArgs(), useClientState() and useRequestState() utils to avoid code duplication.
  • Refactor useQuery(), useSubscription() and useMutation() to use shared utils mentioned above.
  • Add isReadonly() check before changing isPaused variable, as it could be readonly.
  • Remove onBeforeUnmount and unwatch handlers, as watcher automatically stopped when the owner component is unmounted
  • Remove flush: 'pre' in watcher options, as this is default setting
  • Rename unref() to unwrap() to avoid confusions with vue function
  • Reuse then() handler for useQuery() for cleaner code and avoid duplications
  • Cover of reactive arguments usage with test cases

Copy link

changeset-bot bot commented Jun 18, 2024

🦋 Changeset detected

Latest commit: 01c3922

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@urql/vue Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@yurks yurks changed the title feat: refactor composable functions feat(vue): refactor composable functions Jun 19, 2024
@negezor
Copy link
Contributor

negezor commented Jun 20, 2024

This is a really good refactoring. The only thing that bothers me is why the effects in useClientHandle() were initially manually stopped; I couldn’t find the reason (or I didn’t look well).

@yurks
Copy link
Contributor Author

yurks commented Jun 20, 2024

Thanks @negezor ☀️

why the effects in useClientHandle() were initially manually stopped

The reason of having useClientHandle() and manually stopping watchers I found is here #3610 (comment). And the reason it was removed in current PR - I consider this as excessive concern for rare use cases and attempt to bypass vue limitations, which should be completely on users responsibility imho.

But I can agree this kind of removal is overkill for mine "little refactoring", so I'll revert this functionality here.
I guess we can found an optimal solution in scope of that PR.

@negezor
Copy link
Contributor

negezor commented Jun 27, 2024

@kitten could you please review the PR?

@negezor
Copy link
Contributor

negezor commented Jul 12, 2024

@kitten gentle ping 👀

@negezor
Copy link
Contributor

negezor commented Jul 23, 2024

@yurks I deployed these changes to production, found one problem. Uploading files does not work in mutations, it sends {} instead of File

@@ -26,6 +26,12 @@ const unwrap = <T>(maybeRef: MaybeRef<T>): T =>
? maybeRef.value
: maybeRef;

const _toString = Object.prototype.toString;
const isPlainObject = (obj: any): boolean => {
Copy link
Collaborator

@JoviDeCroock JoviDeCroock Jul 25, 2024

Choose a reason for hiding this comment

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

Ï think we might do better in re-using/copying our existing checks https://github.com/urql-graphql/urql/blob/main/packages/core/src/utils/variables.ts#L23-L24 for files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that checks is for Files/Blobs only. I just think this might be overkill here, as finally urql/core is used with its internal validations.
What I am trying to do here - is just unwrap possible refs and pass everything as it is. That's why I found isPlainObject() way more generic: urql/core could be kind of "source of truth" for input validation

@yurks
Copy link
Contributor Author

yurks commented Jul 25, 2024

@yurks I deployed these changes to production, found one problem. Uploading files does not work in mutations, it sends {} instead of File

@negezor thanks for the review! I optimised that piece of code, and added test case for checking different types of variables.

@JoviDeCroock JoviDeCroock merged commit 068df71 into urql-graphql:main Jul 26, 2024
19 checks passed
@github-actions github-actions bot mentioned this pull request Jul 26, 2024
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.

(vue) useQuery pause option is not reactive Possible memory leak when using multiple queries on the same page
3 participants