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

Add ConnectorClientOptions to BotFrameworkAdapterSettings #2013

Closed
EricDahlvang opened this issue Apr 12, 2020 · 4 comments · Fixed by #2081
Closed

Add ConnectorClientOptions to BotFrameworkAdapterSettings #2013

EricDahlvang opened this issue Apr 12, 2020 · 4 comments · Fixed by #2081
Assignees
Labels
P1 Painful if we don't fix, won't block releasing R9 Release 9 - May 15th, 2020
Milestone

Comments

@EricDahlvang
Copy link
Member

Describe the solution you'd like
The ability to set ServiceClientOptions when ConnectorClients are created

Describe alternatives you've considered
Hacking the createConnectorClientInternal method prototype on BotFrameworkAdapter, and accessing the ConnectorClient's properties directly:

BotFrameworkAdapter.prototype.createConnectorClientInternal = function (serviceUrl, credentials) {
    // Set noRetryPolicy: true so ServiceClient code does not add one.
    // This code adds the default retry policies, with a modified exponentialRetryPolicy
    const client = new ConnectorClient(credentials, { baseUri: serviceUrl, noRetryPolicy: true });
    
    //retryCount, retryInterval, minRetryInterval, maxRetryInterval, requestTimeout
    client._requestPolicyFactories.push(retryWithTimeoutPolicy(3, 1000 * 2, 1000 * 1, 1000 * 4, 1000 * 5));
    client._requestPolicyFactories.push(systemErrorRetryPolicy());
    client._requestPolicyFactories.push(throttlingRetryPolicy());

    return client;
};

[enhancement]

@stevengum
Copy link
Member

stevengum commented Apr 14, 2020

Edit:
If we moved to 1.8.13 or 1.8.14 as proposed in #2018 we would get built-in proxy support. By allowing for developers to provide their own HttpClient, we would also get KeepAlive (#183) and custom proxy support. The ability to provide their own HttpClient is enough to make this issue a priority.


This would also get us proxy support and httpClient injection, so I'm all for this.

I believe passing it in via BotFrameworkAdapterSettings should suffice, what do you think @EricDahlvang?

private createConnectorClientInternal(serviceUrl: string, credentials: AppCredentials): ConnectorClient {
    // Helper method for deep clone and setting of serviceUrl and combining/setting USER_AGENT
    // Clones/creates ClientOptions from BotFrameworkAdapter.settings.clientOptions,
    // a new property with a type of ServiceClientOptions
    const clientOptions = this.cloneOrCreateClientOptions(serviceUrl);

    if (BotFrameworkAdapter.isStreamingServiceUrl(serviceUrl)) {
        // Check if we have a streaming server. Otherwise, requesting a connector client
        // for a non-existent streaming connection results in an error
        if (!this.streamingServer) {
            throw new Error(`Cannot create streaming connector client for serviceUrl ${ serviceUrl } without a streaming connection. Call 'useWebSocket' or 'useNamedPipe' to start a streaming connection.`);
        }

        clientOptions.httpClient = clientOptions.httpClient || new StreamingHttpClient(this.streamingServer);

        return new ConnectorClient(credentials, clientOptions);
    }

    return new ConnectorClient(credentials, clientOptions);
}

@stevengum
Copy link
Member

stevengum commented Apr 14, 2020

Using diffchecker, I compared the released 1.2.6 ServiceClientOptions with what's in master (2.x) of @azure/ms-rest-js. The latest from 1.x is the same as master for this interface.

image

image

While there are only additions and union types, we should still guard ourselves against relying on this type.

Guarding against ServiceClientOptions isn't necessary as we have a direct dependency via the subtype ConnectorClientOptions.

@EricDahlvang
Copy link
Member Author

I don't think we can go to 2.0, due to the axios-based to node-fetch change. This would be breaking, since developers are potentially customizing axios in their bot code right now.

@stevengum
Copy link
Member

I'm not advocating for going to 2.0 and I'm aware of that change. The screenshots are a snippet indicating what the ServiceClientOptions delta is.

@stevengum stevengum added the P1 Painful if we don't fix, won't block releasing label Apr 15, 2020
@stevengum stevengum added the R9 Release 9 - May 15th, 2020 label Apr 16, 2020
@munozemilio munozemilio added this to the R9 milestone Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Painful if we don't fix, won't block releasing R9 Release 9 - May 15th, 2020
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants