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

A suggestion regarding the compose method in RequestOptions. #2124

Closed
kajaha2k opened this issue Mar 5, 2024 · 5 comments
Closed

A suggestion regarding the compose method in RequestOptions. #2124

kajaha2k opened this issue Mar 5, 2024 · 5 comments
Labels
i: wontfix This will not be worked on

Comments

@kajaha2k
Copy link

kajaha2k commented Mar 5, 2024

Request Statement

Hello,

I used to use your library without problems,
but I noticed something odd in RequestOptions.
The compose method is supposed to combine options with the base ones,
but I'm wondering why the connection timeout isn't merging properly with the base options."

Would it be possible for my suggestion to better align with the intended meaning of 'compose'?

connectTimeout: baseOpt.connectTimeout,
connectTimeout: connectionTimeout ?? baseOpt.connectTimeout,

 RequestOptions compose(
    BaseOptions baseOpt,
    String path, {
    Object? data,
    Map<String, dynamic>? queryParameters,
    CancelToken? cancelToken,
    ProgressCallback? onSendProgress,
    ProgressCallback? onReceiveProgress,
    StackTrace? sourceStackTrace,
  }) {
    final query = <String, dynamic>{};
    query.addAll(baseOpt.queryParameters);
    if (queryParameters != null) query.addAll(queryParameters);

    final headers = caseInsensitiveKeyMap(baseOpt.headers);
    if (this.headers != null) {
      headers.addAll(this.headers!);
    }
    if (this.contentType != null) {
      headers[Headers.contentTypeHeader] = this.contentType;
    }
    final String? contentType = headers[Headers.contentTypeHeader];
    final extra = Map<String, dynamic>.from(baseOpt.extra);
    if (this.extra != null) {
      extra.addAll(this.extra!);
    }
    final method = (this.method ?? baseOpt.method).toUpperCase();
    final requestOptions = RequestOptions(
      method: method,
      headers: headers,
      extra: extra,
      baseUrl: baseOpt.baseUrl,
      path: path,
      data: data,
      preserveHeaderCase: preserveHeaderCase ?? baseOpt.preserveHeaderCase,
      sourceStackTrace: sourceStackTrace ?? StackTrace.current,
      connectTimeout: baseOpt.connectTimeout,
      sendTimeout: sendTimeout ?? baseOpt.sendTimeout,
      receiveTimeout: receiveTimeout ?? baseOpt.receiveTimeout,
      responseType: responseType ?? baseOpt.responseType,
      validateStatus: validateStatus ?? baseOpt.validateStatus,
      receiveDataWhenStatusError:
          receiveDataWhenStatusError ?? baseOpt.receiveDataWhenStatusError,
      followRedirects: followRedirects ?? baseOpt.followRedirects,
      maxRedirects: maxRedirects ?? baseOpt.maxRedirects,
      persistentConnection:
          persistentConnection ?? baseOpt.persistentConnection,
      queryParameters: query,
      requestEncoder: requestEncoder ?? baseOpt.requestEncoder,
      responseDecoder: responseDecoder ?? baseOpt.responseDecoder,
      listFormat: listFormat ?? baseOpt.listFormat,
      onReceiveProgress: onReceiveProgress,
      onSendProgress: onSendProgress,
      cancelToken: cancelToken,
      contentType: contentType ?? this.contentType ?? baseOpt.contentType,
    );
    requestOptions.cancelToken?.requestOptions = requestOptions;
    return requestOptions;
  }

Solution Brainstorm

No response

@kajaha2k kajaha2k added the s: feature This issue indicates a feature request label Mar 5, 2024
@AlexV525
Copy link
Member

AlexV525 commented Mar 5, 2024

No. The connect timeout is related to the adapter's implementation. Because the adapter holds inner clients to improve requests, the inner client will be used in requests of the same Dio instance, which shares the same connect timeout setting.

@kajaha2k
Copy link
Author

kajaha2k commented Mar 5, 2024

Just as you mentioned, when utilizing an already established connection for improved performance, the connection timeout set during the request may not be crucial. However, in the scenario of a first-time request initiating a new Fetch, wouldn't it be meaningful to set a connectionTimeout during the request?

@AlexV525
Copy link
Member

AlexV525 commented Mar 5, 2024

Changing the connect timeout in the base option will also update the setting in the client.

@kajaha2k
Copy link
Author

kajaha2k commented Mar 5, 2024

Thank you for considering my suggestion.

If it's not problematic or too challenging, reflecting this would be appreciated.

@AlexV525 AlexV525 added i: wontfix This will not be worked on and removed s: feature This issue indicates a feature request labels Mar 6, 2024
@AlexV525
Copy link
Member

AlexV525 commented Mar 6, 2024

I'm marking this as it won't be fixed based on the current design.

@AlexV525 AlexV525 closed this as not planned Won't fix, can't repro, duplicate, stale Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i: wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants