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

Feature request: Pass skip outside of options so you can assume skip is false when evaluating other options #245

Closed
glasser opened this issue Oct 7, 2016 · 7 comments

Comments

@glasser
Copy link
Member

glasser commented Oct 7, 2016

Steps to Reproduce

A common reason that you might want to use skip is that some necessary data for evaluating the query is not available. For example:

const FooWithData = graphql(query, {
  options: ({foo}) => ({
    skip: !foo,
    variables: {
      bar: foo.bar
    }
  })
})(Foo);

Buggy Behavior

However, you can't actually write the code as above: the skip and variables fields are evaluated at the same time, so you have to check foo again in the variables clause.

Expected Behavior

I'd like to be able to instead write:

const FooWithData = graphql(query, {
  skip: ({foo}) => !foo,
  options: ({foo}) => ({
    variables: {
      bar: foo.bar
    }
  })
})(Foo);

Then I can assume while writing the arguments to options that we aren't skipping the query.

Version

@stubailo
Copy link
Contributor

stubailo commented Oct 7, 2016

The same applies to props IMO, which was opened as #212 before.

@glasser
Copy link
Member Author

glasser commented Oct 7, 2016

Note that we might want to keep the old skip around for backwards compatibility.

@jbaxleyiii
Copy link
Contributor

@glasser @stubailo well presented! I'm onboard and will do!

@stubailo
Copy link
Contributor

stubailo commented Oct 7, 2016

I think @glasser is going to give it a shot!

@jbaxleyiii
Copy link
Contributor

jbaxleyiii commented Oct 7, 2016

oh perfect!

A couple thoughts:
What happens if skip becomes true after a query is already being watched?

  • should we unsubscribe?
  • should the old props (data / etc) go away?

We will need to make sure this works on SSR as well

@stubailo
Copy link
Contributor

stubailo commented Oct 7, 2016

I'd say yes and yes - it should be as if the component was I rendered IMO.

@glasser
Copy link
Member Author

glasser commented Oct 7, 2016

@stubailo Actually, between my trouble getting a test environment up (blocking on @tmeasday fixing a test) and my lack of familiarity with the code, I think I'm going to just work around this for now instead of fixing this.

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

No branches or pull requests

3 participants