Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Mutually exclusive "skip" queries now fail #271

Closed
rricard opened this issue Oct 13, 2016 · 9 comments
Closed

Mutually exclusive "skip" queries now fail #271

rricard opened this issue Oct 13, 2016 · 9 comments
Labels

Comments

@rricard
Copy link
Contributor

rricard commented Oct 13, 2016

Steps to Reproduce

See the following failing test: 16f4104

Buggy Behavior

Two graphql wrappers, one skipping, one not skipping can't seem to resolve to the unskipping one.

Expected Behavior

Two mutually exclusive graphql wrappers with skip should be able to resolve the unskipped query.

Version

@rricard rricard added the bug label Oct 13, 2016
@rricard rricard mentioned this issue Oct 13, 2016
6 tasks
@glasser
Copy link
Member

glasser commented Oct 13, 2016

Hmm, I'm confused — when I use two nested graphql I always use the name option to assign their data to two different props. Both of these graphqls are going to assign to data, so I don't really even get how this is supposed to work in a useful way?

@rricard
Copy link
Contributor Author

rricard commented Oct 13, 2016

@glasser worked before and is supposed to work if we have clearly mutually exclusive skips. We leveraged that at different points of our app and worked well so far. I think it is a completely valid use case.

@rricard
Copy link
Contributor Author

rricard commented Oct 13, 2016

Should we support it though ? I'm open to discuss it!

@glasser
Copy link
Member

glasser commented Oct 13, 2016

Yeah, I get why doing the "mutually exclusive skips" makes sense for the actual data part of the data prop, but it does make the "metadata" part (eg loading) a little confused — and even in 0.5.7, skipped queries did set loading: false explicitly...

@rricard
Copy link
Contributor Author

rricard commented Oct 13, 2016

@glasser: that actually explains some other issues we had, however, I still think it's worth fixing: and while we're at it fixing the metadata part as well...

@tmeasday
Copy link
Contributor

This issue will be fixed by #265

But in the meantime you could switch to the new skip API which I believe will do what you want.

@rricard
Copy link
Contributor Author

rricard commented Oct 14, 2016

@tmeasday was intending to do that anyway. But if the test can help you cover this case as well: that's better!

@tmeasday
Copy link
Contributor

tmeasday commented Dec 7, 2016

@rricard is this working for you now? I don't really understand the test you linked to, as:

  1. It uses SSR -- is this relevant?
  2. It seems to expect loading which seems counter to what you want here?

@rricard
Copy link
Contributor Author

rricard commented Dec 7, 2016

  1. No, SSR has nothing to do here
  2. No I want loading, I don't want it masked by the other query

However, as said before, we switched to the new API and named those queries differently, we don't need that fixed anymore. And a few months later, I would advise anyone having this issue to do that as well, trying to feed the data attribute with two mutually exclusive queries is kind of a bad pattern, so, don't do that!

I'll close the issue, if it really really needs fixing, I can try to take a look myself.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants