feat: expose js sdk identity on client metadata#1376
feat: expose js sdk identity on client metadata#1376jonathannorris wants to merge 5 commits intomainfrom
Conversation
Expose stable sdk and framework identity through client metadata so providers and hooks can distinguish web/server usage and framework wrappers without custom vendor shims. Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
There was a problem hiding this comment.
Code Review
This pull request introduces framework-specific metadata (Angular, Nest, and React) and SDK family identification ('web' or 'server') into OpenFeature clients. This is achieved by wrapping clients in a Proxy that intercepts metadata access. The changes include new utility functions for each framework, updates to the respective SDK providers/services to apply these wrappers, and expanded unit tests to verify metadata propagation. Review feedback highlights potential performance improvements in the Angular SDK, specifically regarding redundant client initialization in the directive and the need for caching proxied client instances in the service to avoid overhead during frequent flag evaluations.
Move the framework metadata proxy into shared client utilities so the React, Angular, and Nest SDKs reuse one implementation. Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
|
Hum... should Right now this PR uses I think it probably makes sense to decide whether this field is meant to represent JS package family ( |
Define a minimal core client interface for metadata so shared framework wrappers can depend on an explicit contract instead of a broad object type. Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Reusing makes sense to me. I think they are essentially representing the same thing? The server package sets its |
Keep framework metadata intact while distinguishing JS package family from runtime paradigm, and avoid reinitializing Angular directive clients when the domain input already created one. Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
|
I think this setup probably makes more sense if we ever expand this metadata shape into something more standard across other OpenFeature SDKs, since package identity and runtime paradigm won't necessarily be the same thing in every implementation. |
Align the new directive regression test with the repository's Prettier formatting so the format-lint CI job passes. Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
|
@MattIPv4 @beeme1mr @toddbaert @lukas-reining I put together an alternate implementation in #1377 if you want to compare the two approaches directly. The main difference is that this PR (#1376) uses a Proxy wrapper to inject framework metadata, while #1377 avoids Proxy entirely and instead marks the concrete SDK-owned client instances directly. Both end up exposing the same sdk / paradigm / framework metadata to providers and hooks, but the implementation tradeoff is different: #1376: cleaner encapsulation, but framework identity comes from a wrapper layer Would be good to get feedback on which direction feels cleaner to merge. |
|
👀 Took a look at the alternative, and I think I prefer this approach with a Proxy, as it isolates the metadata change to just where we know for sure the client is being used within that framework -- your other PR mutates the actual client itself, which in the case of at least the React PR, could result in the framework metadata leaking beyond the framework? |
| const stableClient = React.useMemo( | ||
| () => withFrameworkMetadata(client || OpenFeature.getClient(domain), 'react'), | ||
| [client, domain], | ||
| ); |
There was a problem hiding this comment.
One thing I want to make sure we've considered: since @openfeature/web-sdk is a peer dependency of the React (and Angular) SDKs, it's possible for users to import OpenFeature.getClient() directly from @openfeature/web-sdk in a React app, bypassing OpenFeatureProvider (this isn't a problem, we re-export everything, so until now these have always been exactly the same objects and imports, even in terms of instanceof checks, etc - they are literally nominatively the same classes, etc.
However, with this change, there's a difference - in the case above,, sdk and paradigm would be populated, but framework would be undefined.
I think that's arguably correct; if you're not going through the framework wrapper, the metadata should reflect that. But worth calling out since a hook or provider branching on framework === 'react' could get inconsistent results depending on how the client was obtained.
WDYT? Worth a note in the docs/JSDoc, or is this fine as-is?
(Note this impacts both implementations)
toddbaert
left a comment
There was a problem hiding this comment.
After looking at #1377, I think I prefer this, and I don't think there's much risk in the proxy after digging into it. The ref test made me nervous but I don't think it will actually have an impact in anyone's prod code.
Consider this though - not sure if this is a deal-breaker for you @jonathannorris (it's not for me).
Summary
ClientMetadatainto separate JS package identity and runtime identity fieldssdktojs-web/js-server, and addparadigmasclient/serverreact,angular, andnestdomaininput already initialized the clientMotivation
Relates to #1375.
The original version of this PR overloaded
sdkto represent runtime identity. I think it probably makes sense to keep those concerns separate:sdkidentifies the JS package family:js-weborjs-serverparadigmidentifies the runtime model:clientorserverframeworkcontinues to identify wrapper SDKs likereact,angular, andnestThis keeps the metadata more explicit and avoids conflating package family with runtime paradigm.
Usage
Implementation
ClientMetadatanow exposes bothsdkandparadigmProxywrapper so framework metadata is visible both viaclient.metadataand from evaluations that readthis.metadataOpenFeature.getClient()in this SDK returns a new client instance rather than a singletonNotes
domaininput setter, but now avoids the redundant secondinitClient()call fromngOnInit()Related Issues