Skip to content
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

fix(graphql): add query timeout argument when create client. #1447

Conversation

ykuc7
Copy link
Contributor

@ykuc7 ykuc7 commented Jul 19, 2024

Enhancements

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@souravdasslg
Copy link

I needed this functionality, what is the release plan for this ? 🙏

@ykuc7
Copy link
Contributor Author

ykuc7 commented Aug 26, 2024

@vincenzopalazzo
I resolved conflicts. Cloud you approve and merge PR?

@vincenzopalazzo
Copy link
Collaborator

Thanks @ykuc7 but could you remove the merge commit 1b52d19 and just rebase on main

# Conflicts:
#	packages/graphql/lib/src/core/query_manager.dart
#	packages/graphql/lib/src/graphql_client.dart
@ykuc7 ykuc7 force-pushed the fix-add-timeout-argument-to-create-client branch from 1b52d19 to 6180841 Compare August 29, 2024 06:43
@ykuc7
Copy link
Contributor Author

ykuc7 commented Aug 29, 2024

@vincenzopalazzo
Rebased and force pushed.

@vincenzopalazzo vincenzopalazzo merged commit ff0ea7c into zino-hofmann:main Aug 29, 2024
4 checks passed
@uri-ravzin
Copy link

any chance we could have the argument as optional?

e.g. we prefer to let server handle the timeout and let the client library remain as it was before this change :)

@vincenzopalazzo
Copy link
Collaborator

@uri-ravzin PR are welcome if you add a good justification on why you need it optional

@uri-ravzin
Copy link

@uri-ravzin PR are welcome if you add a good justification on why you need it optional

We like to let the server handle the timeout. (as it was in older versions here as well)

short timeouts can lead to repeated requests, unexpected results, and actually increased load on the server, by users who just want to have their requests to complete.

Specifically in our product, we like the upgrades and are fast adopters to all beta releases, but currently seem to be stuck on 5.2.0-beta.7 due to this new introduced mandatory parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants