chore: add node-client-sdk types, options validation, and basic logger#1408
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/browser size report |
|
@launchdarkly/js-client-sdk 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 1834a92. Configure here.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
joker23
force-pushed
the
skz/sdk-2195/node-client-sdk-next-port-client-config-foundation
branch
from
1834a92 to
bbdf442
Compare
This cleans up unused dependencies after we upgraded to eslint 9
| options.destination ?? | ||
| ((line: string) => { | ||
| // eslint-disable-next-line no-console | ||
| console.log(line); |
There was a problem hiding this comment.
Lots of stuff will be needed in the changelog, but I assume the old node client still logged everything as an error? So item for the changelog.
| export interface LDClient extends Omit<LDClientBase, 'identify'> { | ||
| /** | ||
| * Identifies a context to LaunchDarkly and returns a promise which resolves to an object | ||
| * containing the result of the identify operation. |
There was a problem hiding this comment.
I wonder if we need something this being for subsequent contexts and not the initial context for which you use start. (And then also updating others to have similar wording.)
| */ | ||
| start(options?: LDStartOptions): Promise<LDWaitForInitializationResult>; | ||
|
|
||
| /** |
There was a problem hiding this comment.
I think we want more robust wording on this as well.
| * | ||
| * @see https://docs.launchdarkly.com/sdk/features/secure-mode | ||
| */ | ||
| hash?: string; |
There was a problem hiding this comment.
I always forget that we made node a web SDK.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b46eddb. Configure here.
| if (value !== undefined) { | ||
| if (validator.is(value)) { | ||
| // @ts-ignore The type inference has some problems here. | ||
| output[key as keyof ValidatedOptions] = value as any; |
There was a problem hiding this comment.
Null tlsParams passes validation
Medium Severity
validateOptions treats tlsParams: null as valid because it only skips undefined and TypeValidators.Object accepts null in JavaScript. Validated output can then pass null into NodeRequests / EventSource TLS settings instead of falling back or warning.
Reviewed by Cursor Bugbot for commit b46eddb. Configure here.
| tlsParams: TypeValidators.Object, | ||
| enableEventCompression: TypeValidators.Boolean, | ||
| initialConnectionMode: new ConnectionModeValidator(), | ||
| plugins: TypeValidators.createTypeArray('LDPlugin[]', {}), |
There was a problem hiding this comment.
Plugin array allows null entries
Low Severity
The plugins validator uses createTypeArray with an object example, so each array element is only checked with typeof val === 'object'. In JavaScript, typeof null === 'object', so plugins: [null] passes validation without a warning.
Reviewed by Cursor Bugbot for commit b46eddb. Configure here.
joker23
force-pushed
the
skz/sdk-2195/node-client-sdk-next-port-client-config-foundation
branch
from
b46eddb to
cea18dc
Compare
joker23
deleted the
skz/sdk-2195/node-client-sdk-next-port-client-config-foundation
branch
2 participants
Note
Low Risk
Mostly new types, validation, and tests with no full client implementation wired yet; TLS disable warning is intentional security messaging.
Overview
Expands the Node client-side SDK surface with typed public APIs and runtime option handling ahead of wiring up
createClient.NodeOptionsnow extends sharedLDOptionsand addsinitialConnectionMode,plugins,hash, plus existing Node fields (TLS, cache path, event compression). NewvalidateOptionsapplies defaults (e.g. streaming mode, empty plugins), type-checks each Node-specific field with compile-time coverage for new options, warns on bad types, and flags insecuretlsParams.rejectUnauthorized: false.filterToBaseOptionsstrips Node-only keys before the common validator runs.Adds
LDClient(Node-specificidentify,start, connection mode helpers),LDPlugin, and a largeLDCommontype re-export barrel.basicLoggerwraps sharedBasicLoggerwith Nodeconsole+util.format.NodePlatformnow takes aValidatedOptionspick instead of rawNodeOptions. Contract-test ESLint override removed;@types/nodeadded as a dev dependency. Jest tests cover defaults, passthrough, invalid values, TLS warning, and base-option stripping.Reviewed by Cursor Bugbot for commit cea18dc. Bugbot is set up for automated code reviews on this repo. Configure here.