⚠️ No Changeset found

Latest commit: 0af324e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@JoviDeCroock Would you mind taking a look when you have a chance? Thank you!

Hi @JoviDeCroock, I received on email from GitHub on January 23th with a response from you, but I can't see any in this thread, did you delete it?

Just some more info (since you were concerned with the modifications to the tests): basically I reverted the test to their state before 068df71. Many of the test introduced in this commit are testing the deep unwrapping behavior, which is IMO not desirable, that's why I reverted most of it.

I'm not exactly sure how to test that type safety is working, but I can see two solutions:

• using a dependency like tsd (which I'm not familiar with), or
• writing .ts files which would raise errors during TS transpilation if type safety is not working; these tests would be run during tsc step, not vitest though.

Sorry to be a bit pushy about this Issue/PR, but it's been two months and I haven't heard from anyone... Having a broken typing system seems like a major issue for a production-grade GraphQL engine... Even more since it was working before, so people might be relying on it (I did find this bug after pushing some erroneous code which would have been caught if the typing had been working as expected). I noticed other people have run into this issue too.

I did my best to document the issue and propose a solution (I also raised some questions in this PR and the related issue). If that's not good enough or if you are too busy to dig into it right now, could we at least revert the changes from the faulty commit while thinking about a better solution?

@JoviDeCroock @kitten

So it's been 3 months since I posted this PR and the related issue, and I haven't heard from anyone. The regression is 8 months old now and I'm losing hope to see it fixed.

For those interested in having a lightweight vue-compatible graphql client with non-broken typing, I suggest moving to Villus as I did, because it does not seem to be a priority here.

The problems here are several layers deep.

We unfortunately don't use Vue or @urql/vue ourselves in any projects. This means that testing these changes manually, whenever large changes are made, is arduous and takes a long time, so we delay it for as long as possible.
(In this case, I dropped the ball on this entirely, and I'm sorry about that, but usually I'm the one testing these changes)

The other problem here is that this PR needs a changeset, as per the contributing guidelines and the bot comment above: https://github.com/urql-graphql/urql/blob/main/CONTRIBUTING.md#how-do-i-document-a-change-for-the-changelog
Without it, it won't be documented for the release (or flagged for release)

The final problem here is that huge PRs like #3619 with a lot of back and forth and no context on what direction some Vue patterns have moved to cause more problems for us since it compounds the first problem. When we then have PRs that basically introduce a "apply and revert" loop where changes go beyond what's needed in the moment, it increases the surface area of what's checked and lowers my confidence in the change.
For context, the past external PRs to @urql/vue have fixed some issues, but regressed on others, simply because they touched too much code. This PR again makes a huge amount of changes that probably go beyond what's needed.
It also regenerates the lockfile in the same PR, which is a big unrelated change. Gradual updates are important and we're affecting tens of packages here that are affected in a PR that's not for those packages.

Lastly, re. the typings change. We aren't yet making use of extensive typings tests in this repo, since Vitest didn't have that feature when we would've added it. We also haven't added alternatives. This meant more regressions in some patterns than we would've liked and also increases testing burden.

I didn't have the time to write all of this out before, but I hope this is useful nonetheless.
If you'd like to see this quicker, a fork is always safest for yourself and for us in situations like these. I'm still happy to merge this, but I'd have to break down what changes belong to this PR, which changes are correct, what other changes should be split out, etc.
But, yes, villus is the Vue-community maintained library here, and if it suits your need, there's obviously nothing wrong with switching.

Anyway, happy to set aside a few hours to sort this and merge some changes resulting from this, or close this for now, and come back to this another time. Either way works 👍

Thank you for your reply @kitten. I totally understand that you don't have the time to review my PR, especially if you don't use Vue. Sorry if my last message sounded bitter somehow. I really like urql, so I'd really like to help fix this issue eventually.

Just to be clear, I was not expecting you nor anybody else to merge my PR as is (since I explicitly left some points to be discussed). That's why I did not provide a changeset yet. Also, I regenerated the lockfile to pass one of the checks (this one might be quite hazardous).

Now regarding the main issue, I tried to describe extensively the problems and possible workarounds in this PR and in the related issue, but here is a sum up:

• d07602d modified the definition of MaybeRefObj, which caused the typing of variables to be broken
• 068df71 introduced "deep unwrapping" of variables with an incorrect typing (which passes typecheck with a type assertion); I argued in the issue that this behavior is (1) not necessary (it can be achieved with a simple ref or reactive), (2) very hard (if not impossible) to type correctly, (3) not documented.

This is mainly what the PR is about. The rest is just some renaming of utils (to be consistent with Vue current implementations) and reverting the tests that use this deep unwrapping.

I'd really like to have the opinion of someone more knowledgeable about this... Maybe @yurks or @negezor if you have some time to take a look?

Gotcha! Unfortunately, my main problem is that the ref logic is hard for me to get back into. There's a lot of edge cases, and it's on me that I didn't write them down as unit tests. That's also in part due to my lack of experience with Vue.

(On a side-note, we do have some plans re. bindings that may help here, but it's an idea of how to deprecate bindings altogether, so not relevant here, and it'd be a change for the far future)

For now, I don't mind merging this and totally trust you, especially if we can get one more Vue developer to take a glance at this in relation to the past issues (so in addition to #3733, #3641 addressed an edge case, #3633 relates to pause, and #3614 / #3612 may be relevant, among others)
There's a long chain here and maybe we've been too lenient on merging PRs without tests and too idle on supplementing new tests ourselves 🥲

It's also possible new utilities related to an upgrade can help here, since it sounds like we should bump the required version anyway 🤔
#3731 (comment)

I must confess I share some responsibility in that long chain as I did temporarily broke a functionality in #3633 😬
So yes, having at least another Vue dev's opinion on these matters would be great!

Also, I think the Vue bindings documentation should be updated afterwards (for example the functionality I broke was not documented -- and still isn't I think). I'd be happy to help with this too.

I take the opportunity to mention this related (but not vue-specific) question: #3733 (comment)

I may not be getting this right, but for me (and after some test), this type makes variables optional if one of its field is nullable, which does not seem like the expected behavior...

I'm unable to check this right now, but want to mention - deep variables unwrapping was not introduced in #3619. That PR is about fixing memory leaks with unwrapping, but this feature was in place from the point we decided to use this library, and that was about 3 years ago, so this is kind of original functionality.

I'd say removing deep unwrapping is a real breaking change, which costs at least major version bump and deprecation notice.

Personally, I didn't get the point of removing this, but it definitely concerns me not understanding the context. I'll try again a bit later, or kindly ask you for details why you are removing.

Thanks for taking the time to comment!
Seems to me that unwrapDeeply was introduced in this PR (see here)

As to whether this a breaking change or not, I'm not sure, because this behavior has never been documented (to the best of my knowledge). The doc merely states:

All inputs that are passed to useQuery may also be [reactive state](https://v3.vuejs.org/guide/reactivity-fundamentals.html). This means that both the inputs and outputs of useQuery are reactive and may change over time.

My primary concern with unwrapDeeply is that it seems intractable to type. The current typing ((input: T) => T) is wrong (and passes type check with a type assertion). You can check the typing of Vue's reactive (which implements some kind of deep unwrapping of refs) to see that it is already not trivial. And yet, unwrapDeeply does much more, because it also unwrap getters and elements of array (which reactive does not).

My secondary concern is that it is quite a complicated and confusing mechanism, that can be simply achieved by wrapping variables in a reactive or a ref (at least to unwrap the nested refs -- for getters one should use computed or simply unwrap a "shallow" getter). A simple unwrapping would be easier to understand, document, and would cover most use case (I'll write a more detailed example below). As a comparaison, this is how vue-apollo or villus handle variables reactivity.

Here is some expanded explanation about how you can already achieve deep unwrapping with a simple unwrapping (toValue) and Vue built-in utils.

1. reactive (and ref, which uses reactive under the hood) already deeply unwraps references (though not for elements of arrays), so if your variables look like

const variables = {
  foo: someRef,
  bar: someOtherRef,
  baz: {
    a: yetAnotherRef,
    b: andSoOn,
  },
};

you can just pass reactive(variables) or ref(variables) to your query.

2. If you have nested getters, like

const variables = {
  foo: () => someFn(someRef.value),
  bar: () => someOtherRef.value,
  baz: anotherRef
};

you can use a "shallow getter" instead

const variables = () => ({
  foo: someFn(someRef.value),
  bar: someOtherRef.value,
  baz: anotherRef.value
});

Alternatively, you can use computed references, and wrap everything in a reactive or ref, e.g.,

const variables = reactive({
  foo: computed(() => someFn(someRef.value)),
  bar: computed(() => someOtherRef.value),
  baz: anotherRef
});

These examples are merely there to show that you can pass deeply nested reactive variables without implementing some deep unwrapping on urql side. I do understand that in some cases you need to prepare a bit the variables field (wrapping it in ref/reactive, or using computed for more complex cases), but it seems totally acceptable to me (and this is what other vue-compatible GraphQL clients implement)

Ah, I remember this better now. I believe my initial impression here was the same re. the solutions you're mentioning. My main question (as someone who hasn't used Vue since this reactivity system was first added) would be whether that's "ergonomic and established"

If it's not expected for us to deeply unwrap, then we shouldn't do it. As you're saying other clients (Villus?) don't deeply unwrap, right?

However, I suppose that brings us to a very simple conclusion of just a few questions:

• What do other libraries do? (like Villus, Vue Apollo, Tanstack, etc)
• What's expected by most Vue users when they use these bindings?
• What's the difference to Villus apart from this that we may want to adopt?

I'm not against a breaking change, especially since a raised peer dep on vue for the other issue already implies a breaking change.
I'm also not against upholding this decision afterwards (we've clearly accidentally flip-flopped on this, but that's not necessarily an issue — we were relatively early with adopting Vue Reactivity imho, before some preferences were established)

The only concern here is documentation, since they're not in a great shape, and not necessarily in a changeable shape. But we can cross that bridge when we get to it

Edit: I've gone through some past changes and overall I think this holds up. Removing deep unwrapping is straightforward and also what we temporarily upheld originally. Restoring it makes sense, reactive, computed etc usage is expected for these cases, and the docs simply didn't clarify this, and a lack of examples probably made this change too appealing when it was originally merged

Hi! I prefer an approach where we use MaybeRefOrGetter only for variables. In my code, I always write something like this:

useFetchThreadCommentsQuery({
    variables: () => ({
        subjectId: props.subjectId,
        after: afterCursor.value,
        orderBy: orderBy.value,
    }),
});

I got used to this approach back in vue-apollo. However, I'm against wrapping reactive(variables), as this would again lead to memory leaks. Since we're using constants anyway, Vue can attach its proxies to them. This caused me problems in SSR, since each new Vue instance would add new getters, more details #3612.

@negezor Thanks for your feedback! I'll look into PR #3612 to understand the issue (and avoid a possible regression on this matter).

Just to clarify: I did not mean to wrap variables inside reactive on urql side, I just meant that if one wants to unwrap deeply nested refs, one can apply reactive before passing it to useQuery.

Hi @arkandias, thanks for taking the time to work on this! i
In the PR I see you are modifying tests like :

    const sub = useSubscription({
      query: `{ test }`,
    });

to

    const sub = reactive(
      useSubscription({
        query: `{ test }`,
      })
    );

and that's confusing me a little. I was under the impression that your changes were affecting variables only ? Could you clarify ?

I agree with what @negezor is saying, using a MaybeRefOrGetter for variables feels natural and in-line with the most common vue patterns.

That being said, I believe some of the changes here are very good and could be merged much quicker if this PR was split into multiple PRs. Like the renaming of the types to align with vue 3.3 naming.

Hi @Hebilicious, Thanks for taking a look at this! These changes come from the fact that I reverted the tests of useSubscription to before commit #068df71f, but I agree: wrapping useSubscription in a reactive seems odd. Actually, these test are passing just fine as they were, so there is no point reverting them (unlike useQuery: I had to revert some of those because they were testing the deep unwrapping behavior).

What about the query field? Should it be reactive too? If so, I guess MaybeRefOrGetter too? (I never used a reactive query, but maybe it's not so uncommon.)

There is also the implementation note I talked about here, if anyone's interested (I had to use a type assertion at some point).