fix: update LDClient to not require context key for client side identify methods #1045
There are no files selected for viewing
|
Member
There was a problem hiding this comment. 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 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.
Member
There was a problem hiding this comment. Or it could be type Though again I am not sure how good the intellisense for that would be. Maybe fine.
Contributor
Author
There was a problem hiding this comment. I think there is something fundamentally wrong here... so the So given that perhaps the correct thing to do is create an anonymous context interface, eg:
Then ultimately the LDContext that could be used in a client side identify will be: @kinyoklion FYI
Member
There was a problem hiding this comment. 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.
Contributor
Author
There was a problem hiding this comment. 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.
Member
There was a problem hiding this comment. 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.
Member
There was a problem hiding this comment. I do think we need a test that forms all the context in the various permissions as a type of typescript test.
Contributor
Author
There was a problem hiding this comment. 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! |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| import { LDContextCommon, LDSingleKindContext, LDUser } from '@launchdarkly/js-sdk-common'; | ||
|
|
||
| export { LDContext as LDContextStrict } from '@launchdarkly/js-sdk-common'; | ||
|
|
||
| /** | ||
| * @see {@link LDSingleKindContext} | ||
| * | ||
| * The only difference is that the kind cannot be 'multi' which is reserved for multi-kind contexts. | ||
| * Expect this change to be propogated to the common package in the future. | ||
| * | ||
| * @privateRemarks | ||
| * This is helpful for type narrowing to avoid ambiguity when the kind is 'multi'. | ||
| */ | ||
| type strictSingleKindContext = Omit<LDSingleKindContext, 'kind' | 'key'> & { | ||
| key: string; | ||
| kind: Exclude<string, 'multi'>; | ||
| }; | ||
joker23 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * An anonymous version of {@link LDSingleKindContext}. This is a valid form for contexts used in a | ||
| * client-side sdk because the key will be generated if missing by the {@link ensureKey} function. | ||
| */ | ||
| type anonymousSingleKindContext = Omit<LDSingleKindContext, 'key' | 'anonymous' | 'kind'> & { | ||
| key?: string; | ||
| anonymous: true; | ||
| kind: Exclude<string, 'multi'>; | ||
| }; | ||
|
|
||
| /** | ||
| * An anonymous version of {@link LDContextCommon}. This is a valid form for contexts used in a | ||
| * client-side sdk because the key will be generated if missing by the {@link ensureKey} function. | ||
| */ | ||
| type anonymousLDContextCommon = Omit<LDContextCommon, 'key' | 'anonymous'> & { | ||
| key?: string; | ||
| anonymous: true; | ||
| }; | ||
|
|
||
| /** | ||
| * An anonymous version of {@link LDMultiKindContext}. This is a valid form for contexts used in a | ||
| * client-side sdk because the keys will be generated if missing by the {@link ensureKey} function. | ||
| */ | ||
| interface multiKindContextWithAnonymous { | ||
| kind: 'multi'; | ||
| [kind: string]: 'multi' | anonymousLDContextCommon | LDContextCommon; | ||
| } | ||
|
|
||
| /** | ||
| * This is the client side version of the `LDContext` type (referred to as {@link LDContextStrict} in this module). | ||
| * The key reason for this distinction is that client side contexts can be anonymous. | ||
| * An anonymous context is a context that satisfies the following definition: | ||
| * ```typescript | ||
| * { | ||
| * key?: string; | ||
| * anonymous: true; | ||
| * } | ||
| * ``` | ||
| * > NOTE: A context with the `anonymous` property set to `false` or is `undefined` **MUST** have a `key` | ||
| * > property set or it will be rejected by the SDK. | ||
| * | ||
| * Otherwise, refer to {@link LDContextStrict} for more details on how LaunchDarkly contexts work. | ||
| * | ||
| * @see {@link LDSingleKindContext} | ||
| * @see {@link LDMultiKindContext} | ||
| * | ||
| * @remarks | ||
| * Anonymous contexts are acceptable in the client side SDK because the SDK will generate a key for them if they are missing. | ||
| * The key generation logic is in the {@link ensureKey} function. | ||
| */ | ||
| export type LDContext = | ||
| | multiKindContextWithAnonymous | ||
| | strictSingleKindContext | ||
| | anonymousSingleKindContext | ||
| | LDUser; | ||
Uh oh!
There was an error while loading. Please reload this page.