-
Notifications
You must be signed in to change notification settings - Fork 38
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.
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
feat!: Support transient identities and traits #240
Conversation
Awesome, I can see the tests pass so we must be good on regression. I guess we do need additional tests around this:
|
@kyle-ssg I've added:
|
628e45b
to
e0c9881
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
package.json
Outdated
@@ -8,7 +8,8 @@ | |||
"deploy": "npm run build && npm test && cd ./lib/flagsmith/ && npm publish && cd ../../lib/react-native-flagsmith && npm publish && cd ../../lib/flagsmith-es && npm publish", | |||
"deploy:beta": "npm run build && npm test && cd ./lib/flagsmith/ && npm publish --tag beta && cd ../../lib/react-native-flagsmith && npm publish --tag beta && cd ../../lib/flagsmith-es && npm publish --tag beta", | |||
"build": "rollup -c && node ./move-react.js", | |||
"test": "jest --env=jsdom" | |||
"test": "jest --env=jsdom", | |||
"generatetypes": "npm exec quicktype https://raw.githubusercontent.com/Flagsmith/flagsmith/feat/evaluation-context-schema/sdk/evaluation-context.json -- -o evaluation-context.ts --src-lang schema --just-types --prefer-types" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: change branch reference to main
after Flagsmith/flagsmith#4414 is merged.
Rewrote the implementation keeping existing interfaces intact and adding support for the |
e86d808
to
34d9f6a
Compare
As discussed offline, added transiency support to |
|
||
export type TraitEvaluationContext = { | ||
transient?: boolean; | ||
value: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value is actually a IFlagsmithValue, this is important so that people don't send objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sorted out for client input.
evaluation-context.ts
Outdated
[property: string]: any; | ||
} | ||
|
||
export type TraitEvaluationContext = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here we should let people supply just a value or an object. The reason being is that 99% of the time, traits are not transient and would much rather see the following all over my code
traits:{a:1,b:2,c:{value:3, transient: true}}
than
traits:{a:{value:1,transient:false}, b:{}.....}
Also, a lot of people will just blindly send up an object they received from an api or one they've constructed themselves, with this new api they'd have to go through the hassle of converting a key value pair object to what we expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the following ways forward here:
- Modify the common eval context schema to support a union trait type. IMO, this will not improve UX for strictly typed SDKs at all, and the end result will be harder to support.
- Add a
ClientEvaluationContext
type with the union value that will be used in all public interfaces. Envelope the simple trait values internally. Again, more code to support, but slightly more preferable. - Don't do anything with the types but export a utility function for the users to wrap their traits with:
toTraitEvaluationContext(input: { [key: string]: IFlagsmithValue }) => { [key: string]: TraitEvaluationContext }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially I think the following code should work for when (most cases) traits aren't transient
flagsmith.setTraits({ a: 'foo' });
flagsmith.setTrait('a', 'foo');
flagsmith.init({ traits: { a: 'foo' } });
And I guess with evaluationContext
flagsmith.init({ evaluationContext: { identity: { traits: { a: "foo" } }});
As long as the types reflect that option I'm not that concerned about implementation detail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason for this other than less code / easier to read is it means that in most cases we wouldn't have to do some form of conversion of existing objects
I'd have to write some code like the following or manually reconstruct an object
const values = { // some object I also want to pass to Flagsmith
first_name: "Kyle",
last_name: "Johnson",
};
const updatedValues = Object.entries(values).reduce((acc, [key, value]) => {
acc[key] = { value };
return acc;
}, {});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so we're on the same page, the old interfaces, including setTraits
and setTrait
are meant to continue supporting this.
For init
and setContext
/updateContext
, I propose to expose toTraitEvaluationContext
adapter function so users can continue to provide a simple object for traits.
So your example snippet will look like:
import { createFlagsmithInstance, toTraitEvaluationContext } from 'flagsmith';
const values = { // some object I also want to pass to Flagsmith
first_name: "Kyle",
last_name: "Johnson",
};
const flagsmith = createFlagsmithInstance();
await flagsmith.init(...)
await flagsmith.setContext({ identity: { traits: toTraitEvaluationContext(values) } });
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I'm I fan of this, it would add an export that would need to be documented / is not immediately clear what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If toTraitEvaluationContext is always a safe conversion, we may as well hide this complexity away and let the SDK deal with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, essentially, we want an option 2 then:
Add a
ClientEvaluationContext
type with the union value that will be used in all public interfaces. Envelope the simple trait values internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this sort of pattern feels quite familiar having used lots of SDKs in JS / keeps a basic implementation as terse as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this is done.
package.json
Outdated
"test": "jest --env=jsdom" | ||
"build": "npm run generatetypes && rollup -c && node ./move-react.js", | ||
"test": "jest --env=jsdom", | ||
"generatetypes": "npm exec quicktype https://raw.githubusercontent.com/Flagsmith/flagsmith/feat/evaluation-context-schema/sdk/evaluation-context.json -- -o evaluation-context.ts --src-lang schema --just-types --prefer-types --nice-property-names" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't need npm exec since it's a depedency, regardless, this errors for me - same error mentioned here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't get it to work without npm exec
. I'm running npm install --include=dev
. Is there something I'm missing?
f79543d
to
bf76b25
Compare
5c71d8c
to
e915b58
Compare
Closes #236.