-
Notifications
You must be signed in to change notification settings - Fork 93
Missing ssrMode setting in ssr Quick Example / Fix Typescript type #329
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
Conversation
Without ssrMode enabled the system will not dispatch the right Promise on cache-miss.
Added a new Typescript type fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small change otherwise LGTM
packages/graphql-hooks-ssr/README.md
Outdated
@@ -27,7 +27,8 @@ app.get('/', async (req, reply) => { | |||
const client = new GraphQLClient({ | |||
url: 'https://domain.com/graphql', | |||
cache: memCache() // NOTE: a cache is required for SSR, | |||
fetch | |||
fetch, | |||
ssrMode: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't required when using graphql-hooks-ssr
, see https://github.com/nearform/graphql-hooks/blob/master/packages/graphql-hooks-ssr/index.js#L13. Perhaps a comment here to say that graphql-hooks-ssr
set the flag will make it clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will fix pull request
packages/graphql-hooks/index.d.ts
Outdated
@@ -29,7 +29,7 @@ export class GraphQLClient { | |||
options: UseClientRequestOptions | |||
): CacheKeyObject | |||
getFetchOptions(operation: Operation, fetchOptionsOverrides?: object): object | |||
request(operation: Operation, options: object): Promise<Result> | |||
request(operation: Operation, options?: object): Promise<Result> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Sorry, first time margin a pull request into an open source project. Is it better I add a commit reversing the ssrMode change or should I start a new pull request with a clean commit history? |
@helloguille another commit is fine, we always squash the commits when merging to master anyway! |
This reverts commit b8c934b.
Hi: |
@all-contributors please add @helloguille for code |
I've put up a pull request to add @helloguille! 🎉 |
@bmullan91 Thanks for merging! I also authored: #406 |
Without ssrMode enabled the system will not dispatch the right Promise on cache-miss.
What does this PR do?
Updates documentation for ssr.
Checklist