Update typings for identify() function #102
Conversation
| @@ -222,7 +222,7 @@ declare module 'ldclient-js' { | |||
| * @param onDone | |||
| * A callback to invoke after the user is identified. | |||
| */ | |||
| identify: (user: LDUser, hash?: string, onDone?: () => void) => Promise<void>; | |||
| identify: (user: LDUser, hash?: string, onDone?: (err: Error | null, flags: LDFlagSet | null) => void) => Promise<void>; | |||
There was a problem hiding this comment.
I think there's a problem here. Looking at the code, when we call resolve from identify(), the parameter is the response body we got from the requestor. That used to be in the format defined by LDFlagSet (key: value), but now it is the larger response from the evalx endpoint (key: { value, version, etc. }). So LDFlagSet is wrong here, even though it's probably what we should be returning. I'm not sure of the right solution. @apucacao?
There was a problem hiding this comment.
It's possible that non-TS users have already discovered this and are relying on that signature. Given that, can we convert the new response to match LDFlagSet (if possible) and add these definitions?
There was a problem hiding this comment.
But any code that was relying on that signature would not have been working, at least not since we released summary events. So do we break things for people whose code is currently correct, or break things for people who are relying on an out-of-date version?
|
Any updates on this issue? Turning of Typescript type checks when accessing this function is non-ideal. 😄 |
change sendLDHeaders to only affect flag requests, not events
It looks like identify() function's callback is missing arguments definitions:
https://github.com/launchdarkly/js-client/blob/078467e47c37def50030d29176141b908a5539ca/src/index.js#L150
https://github.com/launchdarkly/js-client/blob/078467e47c37def50030d29176141b908a5539ca/src/utils.js#L54