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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

increased strictness of types #695

Merged
merged 16 commits into from May 30, 2017

Conversation

Projects
None yet
@jbaxleyiii
Copy link
Member

jbaxleyiii commented May 7, 2017

This is a PR I'm super excited about. It should make typescript and flow users very happy! 馃帀

Following the discussion in #394, this PR seeks to make adding types easy to the graphql HOC for both flow and typescript

Examples:

Typescript

Type query results and expect their values in the the wrapped component

screenshot 4

Safely use the options function based on props passed to wrapped component

screenshot 8

Safely build props based on result data, passed props, and what react-apollo passes down

reducer-error

Flow

Type query results and expect their values in the the wrapped component

component

Safely use the options function based on props passed to wrapped component

options

Safely build props based on result data, passed props, and what react-apollo passes down

flow-errors

Known flow issues

  1. Options as a function or and object is not currently supported. With union types in flow, I couldn't get an interface with all optional params to be distinguishable from a callable arrow function which returns said interface. For now, only the function version is allowed.
  2. Result data which could be null, is typed as existing. I tried to get result data being optional QueryProps & ?R but then I couldn't get flow to pass something like data.code && // use data.code. If you have any knowledge here I'd love it!

For the flow typings, we will need to move the AC ones to AC and do a release first. I'd love to do a tagged release with them bundled with the lib so they are automatic like typescript typings are. If it works (fingers crossed), we can do the same for RA.

I would love feedback from @helfer @ianks and @brettjurgens if they are willing. This is a breaking typescript change so I've bumped the minor version. I hope that works for people!

I'll probably play around with overloads for typescript to detect mutation / query / subscription. I'm not totally sure how to do that in flow.

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change

James Baxley added some commits May 7, 2017

@brettjurgens
Copy link
Contributor

brettjurgens left a comment

nice 馃憤 some minor comments

}

export default function graphql(
export default function graphql<TResult = {}, TProps = {}, TChildProps = DefaultChildProps<TProps, TResult>>(

This comment has been minimized.

@brettjurgens

brettjurgens May 7, 2017

Contributor

i think you'll need to bump typescript to 2.3 in package.json for the default generics

@@ -142,7 +153,7 @@ export default function graphql(
// However, this is an unlikely scenario.
const recycler = new ObservableQueryRecycler();

class GraphQL extends Component<any, any> {
class GraphQL extends Component<TProps, any> {

This comment has been minimized.

@brettjurgens

brettjurgens May 7, 2017

Contributor

not important, but can probably set state to {}

James Baxley added some commits May 7, 2017

@jbaxleyiii jbaxleyiii changed the title increased strictness of typescript types increased strictness of types May 8, 2017

@stubailo
Copy link
Member

stubailo left a comment

This looks dope!

@@ -4,6 +4,9 @@ Expect active development and potentially significant breaking changes in the `0

### vNext

### 1.4.0
- Feature: Enhanced typescript definitions to allow for more valid type checking of graphql HOC

This comment has been minimized.

@stubailo

stubailo May 8, 2017

Member

I think this will be a breaking change for TS devs, right? Might be worth a mention in the changelog.

This comment has been minimized.

@jbaxleyiii

jbaxleyiii May 8, 2017

Author Member

@stubailo for sure! I'll add that and note it in the release as well

mutate?: MutationFunc<TResult>;
}

export type DefaultChildProps<P, R> = P & { data?: QueryProps & R, mutate?: MutationFunc<R> };

This comment has been minimized.

@stubailo

stubailo May 8, 2017

Member

Wow this is dope

This comment has been minimized.

@jbaxleyiii

jbaxleyiii May 8, 2017

Author Member

right? I'm wondering if I setup some overloads if we can get it to correctly type mutation / query / subscription. Going to try my hand at it tonight

@@ -142,7 +157,7 @@ export default function graphql(
// However, this is an unlikely scenario.
const recycler = new ObservableQueryRecycler();

class GraphQL extends Component<any, any> {
class GraphQL extends Component<TProps, {}> {

This comment has been minimized.

@ianks

ianks May 8, 2017

nit, but I think void would be more appropriate for the type of State here.

skip?: boolean | ((props: any) => boolean);
name?: string;
withRef?: boolean;
shouldResubscribe?: (props: TProps, nextProps: TProps) => boolean;

This comment has been minimized.

@ianks

ianks May 8, 2017

Would props and nextProps not be optional here?

i.e.

{
  //...
  shouldSubscribe: () => true //should be OK
}

This comment has been minimized.

@brettjurgens

brettjurgens May 8, 2017

Contributor

that should still be fine, they should only be optional if the call can pass the variables as undefined

interface Test {
  testFn (x: string, y: string): boolean;
}

let z: Test = { 
  // this is fine
  testFn: () => true
};
@lewisf
Copy link

lewisf left a comment

I only took a look at the Flow typings but generally what are your thoughts about adding more specific constraints on the generics for things that are exported? It may help produce more useful errors when types don't line up in usage.

): void;

declare export function getDataFromTree(
rootElement: any,

This comment has been minimized.

@lewisf

lewisf May 8, 2017

these are consistent with the ts types, but should these be React$Element<*>?

This comment has been minimized.

@jbaxleyiii

jbaxleyiii May 9, 2017

Author Member

@lewisf yeah the server needs better typings all around, I'll update the PR for typescript and flow typings tonight for it

props: P
) => QueryOptions | MutationOptions;

declare export interface OperationOption<TProps, TResult> {

This comment has been minimized.

@lewisf

lewisf May 8, 2017

it may be useful to give any exported things that use generics more specific types

interface OperationOption<TProps: {}, TResult: {}>

declare export interface OperationComponent<
TResult: mixed = {},
TOwnProps: mixed = {},

This comment has been minimized.

@lewisf

lewisf May 8, 2017

Does it make sense to do Object here instead of mixed?

@jbaxleyiii jbaxleyiii self-assigned this May 9, 2017

error?: ApolloError;
networkStatus: number;
loading: boolean;
variables: {
variables?: {

This comment has been minimized.

@brettjurgens

brettjurgens May 9, 2017

Contributor

i think this should not have the ? it looks like if there's no variables it's an empty object, not undefined

@jbaxleyiii

This comment has been minimized.

Copy link
Member Author

jbaxleyiii commented May 10, 2017

This is ready to go for a first release in my opinion after apollographql/apollo-client#1688 is merged and released

@helfer

This comment has been minimized.

Copy link
Member

helfer commented May 12, 2017

This looks great! Let's get that PR on apollo-client merged! 馃帀

@jbaxleyiii

This comment has been minimized.

Copy link
Member Author

jbaxleyiii commented May 12, 2017

@helfer I'll have tests written for flow typings tomorrow so we can merge and release AC, then we can bump the version here, release, write a blog post, then 馃帀

@spicydonuts

This comment has been minimized.

Copy link

spicydonuts commented May 13, 2017

This looks great!

@spicydonuts

This comment has been minimized.

Copy link

spicydonuts commented May 13, 2017

It looks like MutationOptions from react-apollo isn't getting exported because it's already re-exporting everything from apollo-client, which also defines MutationOptions. I was going to make a new issue but it might fit well with these related changes.

@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented May 29, 2017

@jbaxleyiii ping, any more news? Can I help?

@jbaxleyiii

This comment has been minimized.

Copy link
Member Author

jbaxleyiii commented May 30, 2017

@stubailo I wanted to make sure I had the availability to support this release and apollographql/apollo-client#1688 with quick responsiveness after release so I held off until I was certain I could do that. 馃憤

I've updated the AC PR and will tweak this one next to make sure they are ready since I will be able to support the next few releases well!

@jbaxleyiii jbaxleyiii referenced this pull request May 30, 2017

Merged

add initial flow typings for apollo-client #1688

1 of 1 task complete

@jbaxleyiii jbaxleyiii force-pushed the types-rewrite branch from 545241c to 401f604 May 30, 2017

James Baxley added some commits May 30, 2017

James Baxley
James Baxley
James Baxley

@jbaxleyiii jbaxleyiii referenced this pull request May 30, 2017

Merged

Add support for client in options #729

5 of 6 tasks complete

@jbaxleyiii jbaxleyiii merged commit 7cc8d22 into master May 30, 2017

5 checks passed

./dist/index.min.js.gz -28 bytes (-0.24%) 鈫 11,509 bytes
CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on master at 92.219%
Details

@jbaxleyiii jbaxleyiii deleted the types-rewrite branch May 30, 2017

@jpshelley

This comment has been minimized.

Copy link

jpshelley commented Jun 6, 2017

@jbaxleyiii still trying to understand flow, but should types be committed to https://github.com/flowtype/flow-typed as well? So from the command line flow-typed install or flow-typed update can be ran?
My project still shows react-apollo as having flow type any for all its declarations.

@dsifford

This comment has been minimized.

Copy link
Contributor

dsifford commented Jun 7, 2017

Perhaps I'm dense or am lacking sleep, but I can't get this change to work with a combination of decorators, React.PureComponent, and without typing any to the render method of this file:

https://github.com/aliemteam/aliemcards/blob/master/app/cards/Card.tsx

Has this PR been tested enough to merge? I only see a few situations of this being used in the associated test files and the changes are quite complex.

Any help here would be thoroughly appreciated.

Edit: Looks like all the decorator tests are wrapping components that are given the any type (example below). That doesn't seem safe.

@graphql(query)
class Container extends React.Component<any, any> {}
@dsifford

This comment has been minimized.

Copy link
Contributor

dsifford commented Jun 7, 2017

Also, one other thing I suppose I should mention.

I'm using visual studio code. Until now, I've never had a situation where my editor freezes up and lags. Exclusively the new graphql types cause this to happen.

I appreciate the hard work, but IMHO, the types are too strict. Maybe we could work on a happy medium? I'd be happy to collaborate on a PR.

@dsifford

This comment has been minimized.

Copy link
Contributor

dsifford commented Jun 7, 2017

@jbaxleyiii Just pulled the repo, did a yarn install, and opened ./test/react-web/client/graphql/queries/api.test.tsx without touching any code and I'm getting a similar error to what I got in the repo I linked to above.

image

Does this file work for you guys?

@ianks

This comment has been minimized.

Copy link

ianks commented Jun 7, 2017

I appreciate the hard work, but IMHO, the types are too strict.

What does this mean exactly? The types should restrict invalid code, but allow valid code. Can you provide some examples of where this does not happen?

@dsifford

This comment has been minimized.

Copy link
Contributor

dsifford commented Jun 7, 2017

@ianks Maybe a poor choice of words on my part. Strictness is a good thing, I agree. But the complexity of the generics in this PR don't appear to have been tested thoroughly enough. Also, I'm wondering what the added value of adding all this complexity brings to the table?

I admittedly have not used all the functionality of react-apollo, so I can't say with certainty that there isn't added value here, but what I can say is that the strictness I was able to get with the old types was perfectly fine for the features that I was using.

Also, the changes appear to have been crippling to the typescript language server due to how deeply dependent some of the generics are on each other. For example, just tracing the graphql generic quickly, I'm seeing at least 4 unions, 4-5 intersections, and 3-4 levels of sub-generics. It's a great showcase of Typescripts new generic defaults, but it cripples the typescript language server in visual studio code. Change one value inside the graphql function or change a type that is used somewhere in the generic chain, and the language server has to dive into all that complexity every single time to check.

I'm in no way trying to lessen the value of @jbaxleyiii's work on this PR, and really appreciate all the other work he's done, so please don't take this the wrong way. All I'm saying is that it appears that this change could warrant tests in a handful of different situations to make sure that all the added complexity added by these types has been thoroughly tested. Also, I'm wondering if there might be a way to break apart the length of the type dependency chain to make it less of a workload on the typescript language server for those of us who utilize it for real-time warnings (vscode users, for example).

Thanks for considering. Again, happy to help in any way if needed.

@ianks

This comment has been minimized.

Copy link

ianks commented Jun 7, 2017

@dsifford what version of typescript are you on?

@dsifford

This comment has been minimized.

Copy link
Contributor

dsifford commented Jun 7, 2017

@ianks Latest, stable in the project that I linked to above.

2.3.2 when I pulled this repo and opened up ./test/react-web/client/graphql/queries/api.test.tsx and noticed the errors in the screenshot above.

@ianks

This comment has been minimized.

Copy link

ianks commented Jun 7, 2017

@dsifford can you make a minimal reproduction of this? essentially a dir with a yarn.lock and a file which causes this bug?

EDIT: Nevermind I see you provided a file for this

@dsifford

This comment has been minimized.

Copy link
Contributor

dsifford commented Jun 7, 2017

Yep, I'd be glad to! I appreciate the hospitality 馃槃

Gotta run to class now though. Can we hold that thought until maybe tomorrow afternoon?

Also, just to confirm: You aren't able to reproduce the error in this repo that I'm seeing? If not, that's pretty strange... I'd feel pretty silly if this ended up being an issue with my system or something! 馃槚

@alexzielenski

This comment has been minimized.

Copy link

alexzielenski commented Jun 8, 2017

Same errors for me, I thought i was going insane trying to get these types to compile. Can confirm that a simple yarn install on this repo and viewing ./test/react-web/client/graphql/queries/api.test.tsx will yield these errors. On TS 2.3.2, 2.3.4 and 2.4.0

@jbaxleyiii

This comment has been minimized.

Copy link
Member Author

jbaxleyiii commented Jun 12, 2017

@dsifford and @alexzielenski I'm hoping to take a look at your issues this week. Could you please open a new issue for tracking it?

@dsifford

This comment has been minimized.

Copy link
Contributor

dsifford commented Jun 16, 2017

Sorry for the delay @jbaxleyiii! Just opened #786 for this. Thanks in advance! 馃槃

@dav-ell

This comment has been minimized.

Copy link

dav-ell commented Jul 4, 2017

Does documentation exist for how to use this yet? Say, setting up an example project with all the type benefits described in the original post?

The post by @alexzielenski in #786 does a good job of explaining how to get something working, and could be used as a starting point for documentation.

I love what you guys are doing here! Thanks so much for all the hard work!

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