feat: default session id in frontend api #5083
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/lib/services/proxy-service.ts
Dismissed
| @@ -74,12 +74,19 @@ | |||
| const client = await this.clientForProxyToken(token); | |||
| const definitions = client.getFeatureToggleDefinitions() || []; | |||
|
|
|||
| const sessionId = context.sessionId || String(Math.random()); | |||
Check failure
Code scanning / CodeQL
Insecure randomness High
There was a problem hiding this comment.
we're not doing cryptography here
| .map((feature) => ({ | ||
| name: feature.name, | ||
| enabled: Boolean(feature.enabled), | ||
| variant: client.forceGetVariant(feature.name, context), | ||
| variant: client.getVariant(feature.name, { |
There was a problem hiding this comment.
we don't have to use forceGetVariant anymore. Since we have sessionId we don't have to care how getVariant is implemented locally.
|
@sighphyre I can think of a property based test or run a regular test 50 times and verify some properties. It will be more work than the change itself but maybe it's worth the effort. |
Yeah, it may be a lot of work. I also think it may be worth it. A prop test would be awesome. We've done some "run this 10K times and poke at the statistics" tests before, they have been useful but they have drawbacks Either way, I'm not going to block this for not having one of those. I still think LGTM, I'll leave it up to you if you want to that now, later or never |
About the changes
What problem are we solving?
Solution:
Important files
Discussion points
Is Math.random() good enough?