Skip to content

fix: this.identity reverse compatibility #302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: this.identity reverse compatibility #302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
febb6fa
re-add assignment of flagsmith.identity
kyle-ssg Mar 28, 2025
8be3a4a
Version bump
kyle-ssg Mar 28, 2025
ff548b5
fix: rebased-main
Zaimwa9 May 1, 2025
e7c29e6
fix: re-added-identity-in-get-state
Zaimwa9 May 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion flagsmith-core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ const Flagsmith = class {
};
if (identifier) {
this.evaluationContext.identity.identifier = identifier;
this.identity = identifier;
}
}
this.flags = flags;
Expand Down Expand Up @@ -277,6 +278,7 @@ const Flagsmith = class {
flags:IFlags|null= null
getFlagInterval: NodeJS.Timer|null= null
headers?: object | null= null
identity:string|null|undefined = null
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

initialised= false
oldFlags:IFlags|null= null
onChange:IInitConfig['onChange']|null= null
Expand Down Expand Up @@ -439,7 +441,7 @@ const Flagsmith = class {
const onRetrievedStorage = async (error: Error | null, res: string | null) => {
if (res) {
let flagsChanged = null
let traitsChanged = null
const traitsChanged = null
try {
const json = JSON.parse(res) as IState;
let cachePopulated = false;
Expand Down Expand Up @@ -563,6 +565,7 @@ const Flagsmith = class {
}

identify(userId?: string | null, traits?: ITraits, transient?: boolean) {
this.identity = userId
this.evaluationContext.identity = {
identifier: userId,
transient: transient,
Expand Down Expand Up @@ -591,6 +594,7 @@ const Flagsmith = class {
flags: this.flags,
ts: this.ts,
evaluationContext: this.evaluationContext,
identity: this.identity,
evaluationEvent: this.evaluationEvent,
} as IState
}
Expand All @@ -602,11 +606,13 @@ const Flagsmith = class {
this.flags = state.flags || this.flags;
this.evaluationContext = state.evaluationContext || this.evaluationContext,
this.evaluationEvent = state.evaluationEvent || this.evaluationEvent;
this.identity = this.getContext()?.identity?.identifier
this.log("setState called", this)
}
}

logout() {
this.identity = null
this.evaluationContext.identity = null;
if (this.initialised) {
return this.getFlags();
Expand Down Expand Up @@ -676,6 +682,7 @@ const Flagsmith = class {
...evaluationContext,
environment: evaluationContext.environment || this.evaluationContext.environment,
};
this.identity = this.getContext()?.identity?.identifier
Copy link
Contributor

Choose a reason for hiding this comment

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

@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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, good point. Yeah we should add this back too


if (this.initialised) {
return this.getFlags();
Expand Down
3 changes: 3 additions & 0 deletions test/init.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ describe('Flagsmith.init', () => {
);
expect(getStateToCheck(flagsmith.getState())).toEqual({
...identityState,
identity: testIdentityWithTraits,
evaluationContext: {
...identityState.evaluationContext,
identity: {
Expand Down Expand Up @@ -135,7 +136,9 @@ describe('Flagsmith.init', () => {
status: 200,
text: () => fs.readFile(`./test/data/identities_${identityB}.json`, 'utf8'),
});
expect(flagsmith.identity).toEqual(identityA);
await flagsmith.identify(identityB);
expect(flagsmith.identity).toEqual(identityB);
expect(flagsmith.getTrait('a')).toEqual(undefined);
mockFetch.mockResolvedValueOnce({
status: 200,
Expand Down
2 changes: 2 additions & 0 deletions test/test-constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export const defaultState = {
export const testIdentity = 'test_identity'
export const identityState = {
api: 'https://edge.api.flagsmith.com/api/v1/',
identity: testIdentity,
evaluationContext: {
environment: {apiKey: environmentID},
identity: {
Expand Down Expand Up @@ -63,6 +64,7 @@ export const defaultStateAlt = {
export function getStateToCheck(_state: IState) {
const state = {
..._state,
identity: _state.evaluationContext?.identity?.identifier,
};
delete state.evaluationEvent;
// @ts-ignore internal property
Expand Down
1 change: 1 addition & 0 deletions types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export interface IState<F extends string = string> {
evaluationContext?: EvaluationContext;
evaluationEvent?: Record<string, Record<string, number>> | null;
ts?: number;
identity?: string;
}

declare type ICacheOptions = {
Expand Down