-
Notifications
You must be signed in to change notification settings - Fork 279
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
[sitecore-jss] GraphQL Client Retry Improvements #1755
Conversation
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.
Looks nice 👍
Please, take a look at my comments/suggestions below
Some parts can improved/refactored so will be easier to maintain "retries" unit tests moving further
Co-authored-by: Illia Kovalenko <[email protected]>
…ore/jss into feature/jss-943-retry-improvements
packages/create-sitecore-jss/src/templates/nextjs/src/lib/dictionary-service-factory.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Illia Kovalenko <[email protected]>
Co-authored-by: Illia Kovalenko <[email protected]>
Co-authored-by: Illia Kovalenko <[email protected]>
Co-authored-by: Illia Kovalenko <[email protected]>
Co-authored-by: Illia Kovalenko <[email protected]>
Co-authored-by: Illia Kovalenko <[email protected]>
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.
Please, review the new comments I left in our discussions
Co-authored-by: Illia Kovalenko <[email protected]>
…ore/jss into feature/jss-943-retry-improvements
packages/create-sitecore-jss/src/templates/nextjs/src/lib/layout-service-factory.ts
Outdated
Show resolved
Hide resolved
packages/create-sitecore-jss/src/templates/nextjs/src/lib/dictionary-service-factory.ts
Outdated
Show resolved
Hide resolved
…ut-service-factory.ts Co-authored-by: Adam Brauer <[email protected]>
…ionary-service-factory.ts Co-authored-by: Adam Brauer <[email protected]>
Co-authored-by: Adam Brauer <[email protected]>
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.
Looks great 👍Just a couple jsdoc suggestions.
@illiakovalenko FYI me and Adam had a discussion about the error type and we decided to revert back to almost similar implementation which we had in the beginning of this PR. We have introduced a |
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.
The solution using Partial looks good 👍
Added another comment below, after that feel free to merge
Description / Motivation
ECONNRESET
,ETIMEDOUT
andEPROTO
but other codes can be configured using the same.retries
has now been enabled by default with a default value of 3. It can be disabled for all the services by configuring the env variable i.e GRAPH_QL_SERVICE_RETRIES to 0. We can also disableretries
per service by configuring it to 0 inside the service-factory file in the app.Testing Details
Types of changes