fix: server sdk could send user agent headers under a different header name - #1860
fix: server sdk could send user agent headers under a different header name#1860joker23 wants to merge 1 commit into
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/browser size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 91b3508. Configure here.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| import TestLogger from './Logger'; | ||
| import makeCallbacks from './makeCallbacks'; | ||
|
|
||
| describe('LDClientImpl user-agent header name', () => { |
There was a problem hiding this comment.
🟡 New test file groups tests in a wrapper block without any shared setup, against repo test conventions
The new test file wraps three independent tests in a grouping block (describe(...) at packages/shared/sdk-server/__tests__/LDClientImpl.userAgentHeaderName.test.ts:6) even though each test builds its own setup, which the repository's testing rules forbid.
Impact: Test organization diverges from the required repository conventions.
Convention: describe blocks only for shared setup
Both .cursor/rules/testing.mdc and CLAUDE.md state: "Describe blocks should ONLY be used for common setup for a series of tests" and "If there is no shared configuration, do not use a describe block". In this file each of the three it blocks independently calls createBasicPlatform(), sets up the fetch mock, and constructs its own LDClientImpl, so there is no shared setup in the describe. The test names already read as full sentences on their own.
Prompt for agents
The new test file packages/shared/sdk-server/__tests__/LDClientImpl.userAgentHeaderName.test.ts wraps three self-contained tests in a describe block, but no setup is shared between them (each test creates its own platform, fetch mock, and client). Repository rules (.cursor/rules/testing.mdc, CLAUDE.md) say describe blocks should only be used when there is common setup. Either remove the describe block and make the individual test names fully self-describing, or move the common platform/client construction into shared setup inside the describe.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Yeah, I agree with Devin and its dislike of this vacuous testing structure. 😉
There was a problem hiding this comment.
But, really, please change them to only group for shared setup.
| import TestLogger from './Logger'; | ||
| import makeCallbacks from './makeCallbacks'; | ||
|
|
||
| describe('LDClientImpl user-agent header name', () => { |
There was a problem hiding this comment.
Yeah, I agree with Devin and its dislike of this vacuous testing structure. 😉
| import TestLogger from './Logger'; | ||
| import makeCallbacks from './makeCallbacks'; | ||
|
|
||
| describe('LDClientImpl user-agent header name', () => { |
There was a problem hiding this comment.
But, really, please change them to only group for shared setup.
2 participants
This PR will make it so that server side sdks can also determine the header name used to send user agent value. The reason we need this is that the default
user-agentheader name could be blocked by different platforms/configurations which causes us to lose usage data.This is currently delivered as an internal implementation detail to address issues with the shopify oxygen sdk not sending usage data.
Note
Overview
Adds an internal
userAgentHeaderNameoption on the server SDK so the SDK version string can be sent under a custom header instead of the defaultuser-agent, which some platforms block and breaks usage telemetry.LDClientImplthreads this value intodefaultHeadersfor both FDv1 and FDv2 construction paths (events, polling, streaming requestors share the same base headers). When no override is set, behavior staysuser-agent.The Shopify Oxygen SDK sets
x-launchdarkly-user-agentby default so Oxygen runtimes can report usage without relying onuser-agent.New tests cover events flush, polling, default header name, and Oxygen
EventProcessorbase headers.Reviewed by Cursor Bugbot for commit 91b3508. Bugbot is set up for automated code reviews on this repo. Configure here.