-
Notifications
You must be signed in to change notification settings - Fork 18
Adds Flagsmith adapter #103
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
base: main
Are you sure you want to change the base?
Conversation
@tiagoapolo is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
Review the following changes in direct dependencies. Learn more about Socket for GitHub ↗.
|
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.
Thank you for your contributions so far! I left a bit of early guidance
@@ -0,0 +1,64 @@ | |||
# Flags SDK - Flagsmith Provider | |||
|
|||
A provider adapter for the Flags SDK that integrates with [Flagsmith](https://flagsmith.com/), allowing you to use Flagsmith's feature flags and remote configuration in your application. |
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.
Do you work for Flagsmith? If we merge this PR we'd also put up some actual docs on flags-sdk.dev. Would you be wiling to link to our Flags SDK and those docs from https://docs.flagsmith.com?
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.
Hey @dferber90, just jumping in here since Tiago is on vacation - yes, Tiago works for Flagsmith.
In answer to your question, for sure, we'd be happy to link to your docs from https://docs.flagsmith.com - would you be happy with adding reciprocal backlink(s) too?
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.
Great! We'd add you to https://flags-sdk.dev/providers and create a docs page for FlagSmith
47974ab
to
62c9498
Compare
@dferber90 could you take one more look at my PR ? |
@tiagoapolo I‘m out of office this week, but @AAorris will take over leading adapter work and help you get this finalized, listed and released 👍 |
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.
Thanks for your PR! Please consider the following input:
Strongly recommended:
- Support identity through
entities
- Optimize performance for multiple features in one request
Recommended:
- Add origin to link flags to your customers' dashboards
defaultValue, | ||
}): Promise<ValueType> { | ||
await initialize(configParams); | ||
const state = flagsmith.getState(); |
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.
Note that if there are multiple flags, getState()
might be called many times in one request:
a = flag({ key: 'a', adapter: flagSmith.getFeature() })
b = flag({ key: 'a', adapter: flagSmith.getFeature() })
...
await Promise.all([a(), b()])
You may consider ways to reduce duplicate work if it's not deduplicated in your SDK
return { | ||
async decide({ | ||
key, | ||
entities, |
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.
Entities is unused right now, but it's where Flags SDK usually receives user information. I'd expect identity
to be passed to flagsmith.init({})
here if that's how your SDK works
https://docs.flagsmith.com/clients/next-ssr#comparing-ssr-and-client-side-flagsmith-usage
If you expect your customers to pass an identity, you should strongly consider using entities to drive that here.
What I'd expect is something like:
import type { Identity } from 'flagsmith'
// ...
ValueType extends IFlagsmithFeature,
EntitiesType extends IFlagsmithIdentity,
// ...
decide({ entities }) => {
// identity is either entities itself, or a part of it
init({ identity: entities })
}
async function initialize(config: IInitConfig) { | ||
if (flagsmith?.initialised) return; | ||
await flagsmith.init({ fetch: global.fetch, ...configParams }); | ||
} |
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.
It looks like flagsmith may be initialized with a user — In that case, this pattern won't work because we'll never re-initialize for another user. I'll need to better understand how identity
fits in here, and how it will handle the case where there's multiple different users depending on the request.
https://github.com/Flagsmith/flagsmith-js-examples/blob/main/nextjs-middleware/middleware.ts#L15
return { | ||
async decide({ |
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.
Adapters can provide origin
here, which is a hyperlink to the feature flag defined in code. If you can provide this hyperlink, it will allow users to get to the right place in one click from the Flags Explorer
https://flags-sdk.dev/frameworks/next#flags-explorer
Ex. something like
return {
origin: `${configParams.api}/${configParams.environmentId}/feature/${key}`
decide,
}
Co-authored-by: Aaron Morris <[email protected]>
This pull request introduces a new provider adapter for the Flags SDK that integrates with Flagsmith. The changes include adding configuration files, updating dependencies, and creating the necessary code for the adapter.
Flagsmith documentation: https://docs.flagsmith.com/clients/next-ssr
Usage