Skip to content

Conversation

@joker23
Copy link
Contributor

@joker23 joker23 commented Dec 24, 2025

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

sdk-1709

Describe the solution you've provided

  • Changed the identify and identifyResult methods in LDClient to accept LDContextWithAnonymous instead of LDContext, allowing for more flexible context handling.
  • Introduced LDContextWithAnonymous type in a new LDContext.ts file to define contexts that may not have a key.
  • Updated documentation around the anonymous context type to explain that client side sdks will assign a key to a context that does not have one.
  • Updated browser SDK to use new context type for identification functions
  • updated unit tests to ensure that anonymous contexts with undefined keys can be processed

Provide a clear and concise description of what you expect to happen.

Additional context

This change is prompted by seeing the type inconsistencies between v3 and v4 browser sdk. Since we have an ensureKey function that will fedrate a context id to the context if it does not have one, then we should also be able to support passing in contexts that do not have keys.


Note

Type/API updates

  • New packages/shared/sdk-client/src/api/LDContext.ts exports client-side LDContext (supports anonymous contexts without keys) and re-exports strict server type as LDContextStrict.
  • LDClient.getContext() now returns LDContextStrict | undefined across SDKs; event emitter error callback updated accordingly.
  • identify/identifyResult now accept the new client-side LDContext type.

Key implementation changes

  • ensureKey(context, platform) now returns LDContextStrict; generates keys for anonymous single and multi-kind contexts; typings updated throughout.
  • Browser compat/client (LDClientCompatImpl) and shared LDClientImpl imports/uses updated to new types.

Tests

  • Updated tests to pass anonymous contexts without explicit key (removed key: ''), and added cases for multi-kind anonymous contexts.
  • Adjusted imports to use new api/LDContext and verified behavior of ensureKey and identify with anonymous contexts.

Written by Cursor Bugbot for commit 00c6570. This will update automatically on new commits. Configure here.

@joker23 joker23 requested a review from a team as a code owner December 24, 2025 21:00
@github-actions
Copy link
Contributor

github-actions bot commented Dec 24, 2025

@launchdarkly/browser size report
This is the brotli compressed size of the ESM build.
Compressed size: 171236 bytes
Compressed size limit: 200000
Uncompressed size: 798447 bytes

@github-actions
Copy link
Contributor

@launchdarkly/js-sdk-common size report
This is the brotli compressed size of the ESM build.
Compressed size: 25394 bytes
Compressed size limit: 26000
Uncompressed size: 124693 bytes

@github-actions
Copy link
Contributor

github-actions bot commented Dec 24, 2025

@launchdarkly/js-client-sdk-common size report
This is the brotli compressed size of the ESM build.
Compressed size: 19354 bytes
Compressed size limit: 20000
Uncompressed size: 99752 bytes

@github-actions
Copy link
Contributor

github-actions bot commented Dec 24, 2025

@launchdarkly/js-client-sdk size report
This is the brotli compressed size of the ESM build.
Compressed size: 23337 bytes
Compressed size limit: 25000
Uncompressed size: 81335 bytes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this typing looks good for a multi-kind context, but not for a single kind context. In that this typing for a single kind would make key appear to be not an available option.

I wonder if we should just be using basically a copy of the single/multi context kinds with key correct specified as being optional. Then converting that for internal use?

Though, I suppose, it could be an Omit and then & { key?: string }. I want to make sure we have somewhat intelligible code completion though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or it could be type singleKindOptionalKey = Omit<LDSingleKindContextBase, 'key'> | LDSingleKindContextBase

Though again I am not sure how good the intellisense for that would be. Maybe fine.

Copy link
Contributor Author

@joker23 joker23 Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is something fundamentally wrong here... so the ensureKey function will ONLY federate the context with a key if the context is anonymous:

So given that perhaps the correct thing to do is create an anonymous context interface, eg:

interface anonymousSingleKindContext extends Omit<LDSingleKindContextBase, 'key'> {
  key?: string;
  anonymous: boolean;
}

And do the same for multi of course

Then ultimately the LDContext that could be used in a client side identify will be:

type LDContextWithAnonymous =
  | LDSingleKindContext                         // original single kind context
  | anonymousSingleKindContext          // anon context that could be missing key
  | multiKindContextWithAnonymous;

@kinyoklion FYI

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the right path.

We do need to also consider what the impact of this will be consumption side. My reflex is that it should be exported as LDContext, but if not, then we need to make sure that LDContext doesn't upset the structural typing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up going the route of introducing a new type since I think there is still some value in having a context type that can match a context that is "ensured". Typescript is a bit finicky.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be fine, but I think I wasn't super clear in articulating. We certainly want a non-optional type inside the boundaries of the SDK. But I am less certain we want such a type outside the boundaries of the SDK. People already have some difficulty with the types currently exposed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think we need a test that forms all the context in the various permissions as a type of typescript test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see... yea taking a step back and thinking about how we would want users to interact with this SDK, it is more helpful for them to see the context type with an optional key prop. I'll make those changes! Thanks!

@joker23 joker23 force-pushed the skz/sdk-1709/client-sdk-context branch 3 times, most recently from 7cfcaad to 10d285d Compare January 7, 2026 22:04
- Changed the `identify` and `identifyResult` methods in `LDClient` to accept `LDContextPartial` instead of `LDContext`, allowing for more flexible context handling.
- Introduced `LDContextPartial` type in a new `LDContext.ts` file to define contexts that may not have a key.
- Updated documentation around the partial context to explain that client side sdks will assign a key to a context that does not have one.
@joker23 joker23 force-pushed the skz/sdk-1709/client-sdk-context branch from 10d285d to 6cacf7b Compare January 9, 2026 15:55
@joker23 joker23 force-pushed the skz/sdk-1709/client-sdk-context branch from 8c03a5d to 78a6b75 Compare January 13, 2026 16:34
test('ensureKey should create key for an anonymous context in multi', async () => {
const context: LDContext = {
kind: 'multi',
user: { anonymous: true, key: '' },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do probably want 1 test to make sure that an empty key also still works.

@joker23 joker23 force-pushed the skz/sdk-1709/client-sdk-context branch from 9b247e3 to 00c6570 Compare January 14, 2026 17:34
@joker23 joker23 changed the title refactor: update LDClient to use LDContextPartial for identify methods fix: update LDClient to use LDContextPartial for identify methods Jan 14, 2026
@joker23 joker23 changed the title fix: update LDClient to use LDContextPartial for identify methods fix: update LDClient to not require context key for client side identify methods Jan 14, 2026
@joker23 joker23 merged commit 0cf7660 into main Jan 14, 2026
39 of 40 checks passed
@joker23 joker23 deleted the skz/sdk-1709/client-sdk-context branch January 14, 2026 19:59
@github-actions github-actions bot mentioned this pull request Jan 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants