-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: Adds support for REPORT. #575
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.
For reviewers: This is a clone of common/internal/stream/StreamingProcessor with changes to the constructor and start to support using REPORT.
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.
We should make a ticket to move the common one back to server and cleanup any client related changes..
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.
Created SDK-156
this.logger = logger; | ||
this.requests = requests; | ||
this.streamUri = getStreamingUri(dataSourceConfig.serviceEndpoints, path, parameters); | ||
} |
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.
For reviewers: Main changes are in this constructor and start.
// TLS is handled by the platform implementation. | ||
const eventSource = this.requests.createEventSource(this.streamUri, { | ||
headers: this.headers, | ||
...(this.dataSourceConfig.useReport && { method: 'REPORT', body: this.plainContextString }), |
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.
For reviewers: This line is responsible for the usage of REPORT and putting context in the body.
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.
For reviewers: Most of the changes in these test files relate to eliminating the usage of setupMockStreamingProcessor
public readonly useReport = false; | ||
public readonly withReasons = false; | ||
public readonly useReport: boolean = false; | ||
public readonly withReasons: boolean = false; |
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.
For reviewers: Please doublecheck this is a fine change. It does seem to change the typing, but I think these should be boolean.
paths: this.getStreamingPaths(), | ||
tags: this.clientContext.basicConfiguration.tags, | ||
info: this.platform.info, | ||
initialRetryDelayMillis: 1000, |
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.
For reviewers: Changed to take in millis instead of whole seconds for better resolution. This 1 second constant was defined before as a default parameter and it was multiplied like so: initialRetryDelayMillis: 1000 * this.streamInitialReconnectDelay
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.
For reviewers: It looks like this wasn't piped from the config. Updated to use the config. Please verify the streamInitialReconnectDelay
is the correct option.
Updated LDClientImpl.ts from using |
…Processor and using the context in the body of the request.
Need to throw exception when useReport is true but platform does not support custom http methods. Also need to create ticket for adding better handling. |
Done, added logging instead of exception. |
} else { | ||
// no method or body override | ||
methodAndBodyOverrides = {}; | ||
} |
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.
For reviewers: Main changes are in this constructor and start.
Verified application tag is being handled correctly. |
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.
We should make a ticket to move the common one back to server and cleanup any client related changes..
🤖 I have created a release *beep* *boop* --- <details><summary>akamai-edgeworker-sdk-common: 1.2.0</summary> ## [1.2.0](akamai-edgeworker-sdk-common-v1.1.15...akamai-edgeworker-sdk-common-v1.2.0) (2024-09-26) ### Features * Add support for conditional event source capabilities. ([#577](#577)) ([fe82500](fe82500)) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-server-sdk-common bumped from ^2.6.1 to ^2.7.0 </details> <details><summary>akamai-server-base-sdk: 2.1.16</summary> ## [2.1.16](akamai-server-base-sdk-v2.1.15...akamai-server-base-sdk-v2.1.16) (2024-09-26) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/akamai-edgeworker-sdk-common bumped from ^1.1.15 to ^1.2.0 * @launchdarkly/js-server-sdk-common bumped from ^2.6.1 to ^2.7.0 </details> <details><summary>akamai-server-edgekv-sdk: 1.1.16</summary> ## [1.1.16](akamai-server-edgekv-sdk-v1.1.15...akamai-server-edgekv-sdk-v1.1.16) (2024-09-26) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/akamai-edgeworker-sdk-common bumped from ^1.1.15 to ^1.2.0 * @launchdarkly/js-server-sdk-common bumped from ^2.6.1 to ^2.7.0 </details> <details><summary>cloudflare-server-sdk: 2.5.14</summary> ## [2.5.14](cloudflare-server-sdk-v2.5.13...cloudflare-server-sdk-v2.5.14) (2024-09-26) ### Dependencies * The following workspace dependencies were updated * devDependencies * @launchdarkly/js-server-sdk-common-edge bumped from 2.3.9 to 2.4.0 </details> <details><summary>js-client-sdk-common: 1.8.0</summary> ## [1.8.0](js-client-sdk-common-v1.7.0...js-client-sdk-common-v1.8.0) (2024-09-26) ### Features * Add platform support for async hashing. ([#573](#573)) ([9248035](9248035)) * Add support for conditional event source capabilities. ([#577](#577)) ([fe82500](fe82500)) * Add support for js-client-sdk style initialization. ([53f5bb8](53f5bb8)) * Add URLs for custom events and URL filtering. ([#587](#587)) ([7131e69](7131e69)) * Adds support for REPORT. ([#575](#575)) ([916b724](916b724)) * Allow using custom user-agent name. ([#580](#580)) ([ed5a206](ed5a206)) * Implement goals for client-side SDKs. ([#585](#585)) ([fd38a8f](fd38a8f)) * Refactor data source connection handling. ([53f5bb8](53f5bb8)) ### Bug Fixes * Flag store should not access values from prototype. ([#567](#567)) ([fca4d92](fca4d92)) * Use flag value whenever provided even if variaiton is null or undefined. ([#581](#581)) ([d11224c](d11224c)) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-sdk-common bumped from 2.8.0 to 2.9.0 </details> <details><summary>js-sdk-common: 2.9.0</summary> ## [2.9.0](js-sdk-common-v2.8.0...js-sdk-common-v2.9.0) (2024-09-26) ### Features * Add platform support for async hashing. ([#573](#573)) ([9248035](9248035)) * Add support for conditional event source capabilities. ([#577](#577)) ([fe82500](fe82500)) * Add URLs for custom events and URL filtering. ([#587](#587)) ([7131e69](7131e69)) * Adds support for REPORT. ([#575](#575)) ([916b724](916b724)) * Allow using custom user-agent name. ([#580](#580)) ([ed5a206](ed5a206)) * Implement goals for client-side SDKs. ([#585](#585)) ([fd38a8f](fd38a8f)) ### Bug Fixes * Multi-kind context containing only 1 kind conveted incorrectly. ([#594](#594)) ([b6ff2a6](b6ff2a6)) </details> <details><summary>js-server-sdk-common: 2.7.0</summary> ## [2.7.0](js-server-sdk-common-v2.6.1...js-server-sdk-common-v2.7.0) (2024-09-26) ### Features * Add platform support for async hashing. ([#573](#573)) ([9248035](9248035)) * Add support for conditional event source capabilities. ([#577](#577)) ([fe82500](fe82500)) * Allow using custom user-agent name. ([#580](#580)) ([ed5a206](ed5a206)) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-sdk-common bumped from 2.8.0 to 2.9.0 </details> <details><summary>js-server-sdk-common-edge: 2.4.0</summary> ## [2.4.0](js-server-sdk-common-edge-v2.3.9...js-server-sdk-common-edge-v2.4.0) (2024-09-26) ### Features * Add support for conditional event source capabilities. ([#577](#577)) ([fe82500](fe82500)) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-server-sdk-common bumped from 2.6.1 to 2.7.0 </details> <details><summary>node-server-sdk: 9.6.0</summary> ## [9.6.0](node-server-sdk-v9.5.4...node-server-sdk-v9.6.0) (2024-09-26) ### Features * Add support for conditional event source capabilities. ([#577](#577)) ([fe82500](fe82500)) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-server-sdk-common bumped from 2.6.1 to 2.7.0 </details> <details><summary>node-server-sdk-dynamodb: 6.1.22</summary> ## [6.1.22](node-server-sdk-dynamodb-v6.1.21...node-server-sdk-dynamodb-v6.1.22) (2024-09-26) ### Dependencies * The following workspace dependencies were updated * devDependencies * @launchdarkly/node-server-sdk bumped from 9.5.4 to 9.6.0 * peerDependencies * @launchdarkly/node-server-sdk bumped from >=9.4.3 to >=9.6.0 </details> <details><summary>node-server-sdk-otel: 1.0.14</summary> ## [1.0.14](node-server-sdk-otel-v1.0.13...node-server-sdk-otel-v1.0.14) (2024-09-26) ### Dependencies * The following workspace dependencies were updated * devDependencies * @launchdarkly/node-server-sdk bumped from 9.5.4 to 9.6.0 * peerDependencies * @launchdarkly/node-server-sdk bumped from >=9.4.3 to >=9.6.0 </details> <details><summary>node-server-sdk-redis: 4.1.22</summary> ## [4.1.22](node-server-sdk-redis-v4.1.21...node-server-sdk-redis-v4.1.22) (2024-09-26) ### Dependencies * The following workspace dependencies were updated * devDependencies * @launchdarkly/node-server-sdk bumped from 9.5.4 to 9.6.0 * peerDependencies * @launchdarkly/node-server-sdk bumped from >=9.4.3 to >=9.6.0 </details> <details><summary>react-native-client-sdk: 10.7.0</summary> ## [10.7.0](react-native-client-sdk-v10.6.1...react-native-client-sdk-v10.7.0) (2024-09-26) ### Features * Add support for conditional event source capabilities. ([#577](#577)) ([fe82500](fe82500)) * Add support for js-client-sdk style initialization. ([53f5bb8](53f5bb8)) * Adds support for REPORT. ([#575](#575)) ([916b724](916b724)) * Refactor data source connection handling. ([53f5bb8](53f5bb8)) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-client-sdk-common bumped from 1.7.0 to 1.8.0 </details> <details><summary>vercel-server-sdk: 1.3.17</summary> ## [1.3.17](vercel-server-sdk-v1.3.16...vercel-server-sdk-v1.3.17) (2024-09-26) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-server-sdk-common-edge bumped from 2.3.9 to 2.4.0 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- <details><summary>js-client-sdk: 0.0.1</summary> ## 0.0.1 (2024-10-10) ### Features * Add basic secure mode support for browser SDK. ([#598](#598)) ([3389983](3389983)) * Add bootstrap support. ([#600](#600)) ([4e5dbee](4e5dbee)) * Add browser info. ([#576](#576)) ([a2f4398](a2f4398)) * Add ESM support for common and common-client (rollup) ([#604](#604)) ([8cd0cdc](8cd0cdc)) * Add support for browser contract tests. ([#582](#582)) ([38f081e](38f081e)) * Add support for hooks. ([#605](#605)) ([04d347b](04d347b)) * Add support for js-client-sdk style initialization. ([53f5bb8](53f5bb8)) * Add support for localStorage for the browser platform. ([#566](#566)) ([4792391](4792391)) * Add URLs for custom events and URL filtering. ([#587](#587)) ([7131e69](7131e69)) * Add visibility handling to allow proactive event flushing. ([#607](#607)) ([819a311](819a311)) * adds datasource status to sdk-client ([#590](#590)) ([6f26204](6f26204)) * Adds support for REPORT. ([#575](#575)) ([916b724](916b724)) * Browser-SDK Automatically start streaming based on event handlers. ([#592](#592)) ([f2e5cbf](f2e5cbf)) * Implement browser crypto and encoding. ([#574](#574)) ([e763e5d](e763e5d)) * Implement goals for client-side SDKs. ([#585](#585)) ([fd38a8f](fd38a8f)) * Implement support for browser requests. ([#578](#578)) ([887548a](887548a)) * Refactor data source connection handling. ([53f5bb8](53f5bb8)) * Scaffold browser client. ([#579](#579)) ([0848ab7](0848ab7)) ### Bug Fixes * Ensure browser contract tests run during top-level build. ([#589](#589)) ([7dfb14d](7dfb14d)) * Ensure client logger is always wrapped in a safe logger. ([#599](#599)) ([980e4da](980e4da)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Requirements
Related issues
251674
Describe the solution you've provided
StreamingProcessor
constructor.StreamingProcessor
and associated tests from common-internal to sdk-client. Refactored it to support taking in aStreamingDataSourceConfig
to narrow provided dependencies instead of ClientContext which contained many concerns.StreamingProcessor
now uses a paths provider to form URI and internally decides where to put the serialized context (url vs body).Describe alternatives you've considered
Initially tried making changes in common-internal, but the existence of the client context in the URL or body for client situations made that not ideal.