fix: this.identity reverse compatibility #302
Conversation
| @@ -269,6 +270,7 @@ const Flagsmith = class { | |||
| flags:IFlags|null= null | |||
| getFlagInterval: NodeJS.Timer|null= null | |||
| headers?: object | null= null | |||
| identity:string|null|undefined = null | |||
There was a problem hiding this comment.
How are null and undefined values different? I see we're setting this to null on logout, but is that different at all from being undefined?
If they're the same could simplify this to string | undefined?
There was a problem hiding this comment.
It's a bit of a "not yet known" versus we explicitely know there are no value. I personally thinks it's better for readability to keep and set to null on purpose when logging out and keep the undefined when initing.
To represent a bit the:
undefined => loading while detecting if null or existing
string => has identity
null => no identity for sure
| @@ -666,6 +671,7 @@ const Flagsmith = class { | |||
| ...evaluationContext, | |||
| environment: evaluationContext.environment || this.evaluationContext.environment, | |||
| }; | |||
| this.identity = this.getContext()?.identity?.identifier | |||
There was a problem hiding this comment.
@kyle-ssg , I saw that identity got removed from the state there. Just want to make sure that for reverse compatibility it wouldn't need flagsmith.getState().identity
#240 aimed to introduce a new api for setting client context such as identity/traits whilst being backwards compatible with the current API. This however did introduce a breaking change where flagsmith.identity does not reflect the current context.
Until we make the leap to documenting contexts / suggesting this as the approach to using the SDK, this PR re-adds this for backwards compatibility.