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.

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

Adding 'skip' prop to query and mutation components and hooks. #229

Closed
wants to merge 1 commit into from
Closed

Adding 'skip' prop to query and mutation components and hooks. #229

wants to merge 1 commit into from

Conversation

matheus-rosin
Copy link

Hi!

So the idea is to have a prop that can tell Query and Mutation components and useQuery and useMutation hooks, "hey, I know I'm asking you to execute the request, but because of something, don't do that now -- skip this for me".

Skip must be a boolean (not making something like !!skip).
It won't prevent creating a new request; createRequest will be called anyway. This behaviour is due to createRequest not being responsible for checking if the request must be made or not; it only makes a request and returns it. Checking is Query, useQuery, Mutation and useMutation business.

Usage on Query (as a prop on Query component):

import React from 'react';
import { Query } from 'urql';

const getTodos = `
  query GetTodos($limit: Int!) {
    todos(limit: $limit) {
      id
      text
      isDone
    }
  }
`;

const TodoList = ({ limit = 10 }) => {
  let skip = true; // here should have some checking
  return (
    <Query query={getTodos} variables={{ limit }} skip={skip}>
      {({ fetching, data, error }) => {
        if (fetching) {
          return 'Loading...';
        } else if (error) {
          return 'Oh no!';
        }

        return (
          <ul>
            {data.todos.map(({ id, text }) => (
              <li key={id}>{text}</li>
            ))}
          </ul>
        );
      }}
    </Query>;
  );
};

Usage on Mutation (as second parameter of executeMutation):

import React, { Component } from 'react';
import { Mutation } from 'urql';

const addTodo = `
  mutation AddTodo($text: String!) {
    addTodo(text: $text) {
      id
      text
    }
  }
`;

class TodoForm extends Component {
  state = {
    error: null,
    skip: true, // here should have some checking
  };

  add = () => {
    this.props.addTodo({ text: 'something!' }, this.state.skip)
      .catch(error => {
        this.setState({ error });
      });
  };

  render() {
    if (this.state.error) {
      return 'Oh no!';
    }

    return <button onClick={this.add}>Add something!</button>
  }
}

const WithMutation = () => (
  <Mutation query={addTodo}>
    {({ executeMutation }) => <TodoForm addTodo={executeMutation} />
  </Mutation>
);

Usage on useQuery (as as prop of the object passed to the hook):

import React from 'react';
import { useQuery } from 'urql';

const getTodos = `
  query GetTodos($limit: Int!) {
    todos(limit: $limit) {
      id
      text
      isDone
    }
  }
`;

const TodoList = ({ limit = 10 }) => {
  const [res] = useQuery({
    query: getTodos,
    variables: { limit },
    skip: true, // here should have some checking
  });

  if (res.fetching) {
    return 'Loading...';
  } else if (res.error) {
    return 'Oh no!';
  }

  return (
    <ul>
      {res.data.todos.map(({ id, text }) => (
        <li key={id}>{text}</li>
      ))}
    </ul>
  );
};

Usage on useMutation (as second parameter of executeMutation):

import React, { useCallback } from 'react';
import { useMutation } from 'urql';

const addTodo = `
  mutation AddTodo($text: String!) {
    addTodo(text: $text) {
      id
      text
    }
  }
`;

const TodoForm = () => {
  const [res, executeMutation] = useMutation(addTodo);
  const skip = true; // here should have some checking

  if (res.error) {
    return 'Oh no!';
  }

  return (
    <button onClick={() => executeMutation({ text: 'something!' }, skip)}>
      Add something!
    </button>
  );
};

This behaviour is great because sometimes we can notice that we don't have something that we need for making a request, and don't wanna return null, be it through a component itself or through a HOC.

If any of these starts with skip = true, it'll hold its state as it was initialized ({ data: undefined, error: undefined, fetching: false }).

I'm out of time for writing tests. Sorry for that.

Thank you for your attention! 😸

Copy link
Member

@kitten kitten left a comment

Choose a reason for hiding this comment

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

Really neat addition! Some issues though 😅

  • We won't need skip on mutations since they're imperative, so the executeMutation handles can simple not be called to achieve the same
  • We need to set fetching to false and unsubscribe to clean up after ourselves
  • updates aren't triggered when skip changes in both the hook and the component

Thanks for kickstarting this 🙌💕

@@ -20,7 +20,8 @@ interface MutationHandlerState {

interface MutationChildProps extends MutationHandlerState {
executeMutation: <T = any, V = object>(
variables?: V
variables?: V,
skip?: boolean
Copy link
Member

Choose a reason for hiding this comment

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

it doesn't really make sense to add skip to mutations as they're always methods that are called imperatively

@@ -26,6 +27,8 @@ class QueryHandler extends Component<QueryHandlerProps, QueryHandlerState> {
private request = createRequest(this.props.query, this.props.variables);

executeQuery = (opts?: Partial<OperationContext>) => {
if (this.props.skip) return;
Copy link
Member

Choose a reason for hiding this comment

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

When the props change we do call executeQuery again at which point we have to tear down the previous query.

This means that unsubscribe should still be called when skip is true

The update life cycle method will also have to trigger when skip changes (componentDidUpdate)

@@ -26,7 +26,9 @@ export const useMutation = <T = any, V = object>(
data: undefined,
});

const executeMutation = (variables?: V) => {
const executeMutation = (variables?: V, skip?: boolean) => {
if (skip) return;
Copy link
Member

Choose a reason for hiding this comment

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

Same as the component; it's imperative so the addition doesn't really make sense 😀

@@ -38,6 +39,8 @@ export const useQuery = <T = any, V = object>(

const executeQuery = useCallback(
(opts?: Partial<OperationContext>) => {
if (args.skip) return;
Copy link
Member

Choose a reason for hiding this comment

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

Same as the component, we'll still have to call unsubscribe. It might also make sense to set fetching to false when this happens

@matheus-rosin
Copy link
Author

I'm sorry for the late. Soon I'll read all of this :)

@kitten
Copy link
Member

kitten commented Apr 30, 2019

@matheus-rosin no worries at all! Thanks for taking the time to open a PR in the first place!

Let me know if you can't find the time to finish this up, if push comes to shove

@matheus-rosin
Copy link
Author

Man, I think I won't be able to continue that... I'm completely out of time

@kitten
Copy link
Member

kitten commented May 6, 2019

@matheus-rosin No worries at all! I'll pick this up then

@parkerziegler
Copy link
Contributor

Closing per #237.

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.

3 participants