adamsoffer / next-apollo

React higher-order component for integrating Apollo Client with Next.js
MIT License
482 stars 64 forks source link

TypeScript support (#71) #81

Closed ecklf closed 3 years ago

ecklf commented 3 years ago

closes #71

Before merging I would like to talk about the following lines that needed type overwrites:

1) https://github.com/impulse/next-apollo/blob/master/src/withApollo.tsx#L58 It seems there is no toJSON() method. Is this still relevant for apollo client v3?

2) https://github.com/impulse/next-apollo/blob/master/src/withApollo.tsx#L220 It seems ssrMode cannot be accessed from an ApolloClient instance? Wouldn't it be the better solution to do:

const apolloClient = new ApolloClient({
+ ssrMode: typeof window === "undefined",
  uri: "https://api.graph.cool/simple/v1/cixmkt2ul01q00122mksg82pn",
  cache: new InMemoryCache()
});

3) https://github.com/impulse/next-apollo/blob/master/src/withApollo.tsx#L182 Maybe someone knows how to address this type error.

Result in VSCode: image

adamsoffer commented 3 years ago

Wow thanks @impulse ! This looks great. I'm on vacation at the moment but I'll get try and review and test this ASAP.

adamsoffer commented 3 years ago

Hey @impulse. This looks great. Regarding your questions...

It seems there is no toJSON() method. Is this still relevant for apollo client v3?

Not sure TBH

It seems ssrMode cannot be accessed from an ApolloClient instance?

Your solution LGTM

Maybe someone knows how to address this type error.

Not sure about this one either.

I'm good to merge this and publish whenever you address the above. Thanks again for this epic PR.

ecklf commented 3 years ago

@adamsoffer I think we can merge this for now in the scope of converting things to TypeScript. I don't want to introduce anything breaking, so I guess it would be better to work out and extensively test the things I've mentioned in future PRs.

TheBit commented 3 years ago

Thanx guys!