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

Added variables types with Typescript #997

Merged
merged 3 commits into from Sep 22, 2017

Conversation

Projects
None yet
4 participants
@TiuSh
Copy link
Contributor

TiuSh commented Aug 17, 2017

Here is a small update on Typescript part, so we can type queries' & mutations' variables, as apollo-codegen generates these types for us. Default values are used, so it shouldn't break any previous code.

I also added some tests to describe general usage.

@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Aug 17, 2017

@TiuSh: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Aug 24, 2017

@TiuSh This is a great addition! Thank you! I'll figure out what is going on with the CI and merge it in!

Congrats on your first contribution here!

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Aug 24, 2017

@TiuSh can you rebase this on master?

@TiuSh TiuSh force-pushed the TiuSh:typescript-mutation-variables-typing branch from 27770ec to f171298 Aug 25, 2017

@srghma

This comment has been minimized.

Copy link

srghma commented Sep 7, 2017

Awesome pr, just thought about it too, cant wait 👍

@TiuSh TiuSh force-pushed the TiuSh:typescript-mutation-variables-typing branch from f171298 to 6494d28 Sep 7, 2017

@TiuSh

This comment has been minimized.

Copy link
Contributor Author

TiuSh commented Sep 7, 2017

@jbaxleyiii I rebased my code on master, and even added a changelog but the build is still failing...

@srghma

This comment has been minimized.

Copy link

srghma commented Sep 11, 2017

@TiuSh is it possible to make options -> variables more typed?

For example:
I have following query

query allPosts($first: Int!, $skip: Int!) {
  allPosts(orderBy: createdAt_DESC, first: $first, skip: $skip) {
    id
    title
    votes
    createdAt
  }
  _allPostsMeta {
    count
  }
}

and typings generated for it with apollo-codegen

/* tslint:disable */
//  This file was automatically generated and should not be edited.

export type allPostsQueryVariables = {
  first: number
  skip: number
}

export type allPostsQuery = {
  allPosts: Array<{
    id: string
    title: string
    votes: number | null
    createdAt: string | null
  }>
  _allPostsMeta: {
    count: number
  }
}
/* tslint:enable */

what I want is to make use of allPostsQueryVariables

import { graphql } from 'react-apollo'
import { allPostsQuery, allPostsQueryVariables } from '~/schema'
import allPostsGql from './allPosts.gql'

const POSTS_PER_PAGE = 10

const withData = graphql<allPostsQuery>(allPostsGql, {
  options: {
    variables: {
      first: POSTS_PER_PAGE,
      skip: '0', // must throw error, cause skip is number
    },
  },
})

export default withData
@TiuSh

This comment has been minimized.

Copy link
Contributor Author

TiuSh commented Sep 12, 2017

Yes, it would be a little cumbersome, but I think it should be possible to type it a little more, so you could do something like :

import { graphql, ChildProps } from 'react-apollo'
import { allPostsQuery, allPostsQueryVariables } from '~/schema'
import allPostsGql from './allPosts.gql'

const POSTS_PER_PAGE = 10

type myProps = {};

const withData = graphql<allPostsQuery, myProps, ChildProps<myProps, allPostsQuery>, allPostsQueryVariables>(allPostsGql, {
  options: {
    variables: {
      first: POSTS_PER_PAGE,
      skip: '0', // will throw error, cause skip is number
    },
  },
})

export default withData

I'll take a look if i can add it to this PR !

@srghma

This comment has been minimized.

Copy link

srghma commented Sep 17, 2017

@TiuSh yep, it would be awesome

for now using this workaround

import { graphql, QueryProps } from 'react-apollo'

import { AllPostsQuery } from '~/graphql'
import { mergeProps } from '~/utils/ramda-ext'

import allPostsGql from './allPosts.graphql'

export type Response = AllPostsQuery.Result
export type WrappedProps = Response & QueryProps
export type InputProps = {}

const POSTS_PER_PAGE = 10

const variables: AllPostsQuery.Variables = {
  skip: 0,
  first: POSTS_PER_PAGE,
}

const withData = graphql<Response, InputProps, WrappedProps>(allPostsGql, {
  options: { variables },
  props: mergeProps(['data']),
})

export const withApollo = withData
@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Sep 22, 2017

@TiuSh I'm so excited about this! Thank you! I'll see if I can figure out what is up the CI and get this released

@jbaxleyiii jbaxleyiii merged commit edaacf9 into apollographql:master Sep 22, 2017

4 checks passed

CLA Author has signed the Meteor CLA.
Details
Danger All green. Yay.
Details
bundlesize ./dist/index.min.js: 11.95kB < maxSize 12kB gzip
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

srghma pushed a commit to srghma/react-apollo that referenced this pull request Sep 22, 2017

jbaxleyiii pushed a commit that referenced this pull request Sep 22, 2017

@srghma

This comment has been minimized.

Copy link

srghma commented Sep 22, 2017

Bad that TVariables cant be configured in graphql method (its related to this comment)

P.S. revert was accidental)

jbaxleyiii pushed a commit that referenced this pull request Sep 22, 2017

James Baxley
Revert "Upgraded to 0.54.1 Flow" (#1125)
* Revert "Added variables types with Typescript (#997)"

This reverts commit edaacf9.

* Revert "Upgraded to 0.54.1 Flow (#1002)"

This reverts commit c2b3521.
@TiuSh

This comment has been minimized.

Copy link
Contributor Author

TiuSh commented Sep 27, 2017

Hey @jbaxleyiii thank your for the merge !

And sorry @BjornMelgaard, I was in vacations, I couldn't make the change in this pull request... But I'll try to create another pr with this update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment