-
Notifications
You must be signed in to change notification settings - Fork 0
feat: make toolbar compatible with js client sdk v4 #459
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
433d561 to
5d0f08d
Compare
5d0f08d to
8402b84
Compare
|
|
||
| // Subscribe to changes with incremental updates | ||
| const handleChange = (changes: Record<string, { current: any }>) => { | ||
| // NOTE: we are overloading this function so that it can handle both the old and new browser SDKs |
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.
REVIEWER: we'll need to make sure we make super clear what is going on here. Am open to suggestions on what is more standard practice to support different event payloads.
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 really would work better with an interface like inspectors, but it isn't convenient with the plugins.
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 thinking about this more... This is probably the only place that needs to change to make the toolbar compatible with js client sdk v4 (as well as making sure we have a compatible type def)... So there are 2 options that I am thinking:
- Make the current implementation a bit better by doing some kind of type checking (ultimately we would know that it is a v4 change payload if the second param is a
string[] - We can also maybe leverage the client metadata (https://launchdarkly.github.io/js-core/packages/sdk/browser/docs/interfaces/LDPluginApplicationMetadata.html), but I guess that depends on whether it is guaranteed to exist in all versions?
c9348ce to
e6738af
Compare
df4936a to
3d36bb0
Compare
|
putting this back into draft mode until we either stabilize the js client sdk OR have a pre-release stream to merge to. |
9eef50a to
973abb4
Compare
|
@launchdarkly/team-fm-next this PR is ready for review... the approach here is to ensure that the toolbar is compatible with both the new and old js client sdks |
| */ | ||
| export interface LDClient { | ||
| track(key: string, data?: any, metricValue?: number): void; | ||
| identify(ctx: any): Promise<LDIdentifyResult> | Promise<LDFlagSet> | Promise<void>; |
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.
We can use the LDContext type here.
| identify(ctx: any): Promise<LDIdentifyResult> | Promise<LDFlagSet> | Promise<void>; | |
| identify(ctx: LDContext): Promise<LDIdentifyResult> | Promise<LDFlagSet> | Promise<void>; |
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 problem with using LDContext directly here is that it conflicts with the type definition in launchdarkly-react-client-sdk which is very old at this point... structurally, it is hard to compat launchdarkly-react-client-sdk, launchdarkly-js-client-sdk and @launchdarkly/react-client-sdk which is why I put the compat client to accept any... we can probably still maintain type safety if we narrow the context when it is being used?
We are looking at updating the React SDK soon so it should be more compatible in the future.
I don't think this suggestion will build.
packages/toolbar/src/core/ui/Toolbar/context/FlagSdkOverrideProvider.tsx
Outdated
Show resolved
Hide resolved
|
Thanks for doing this @joker23 🙏 It's looking good and I've left a couple of minor suggestions. I do have a couple of questions:
|
|
Thanks @pranjal-jately-ld for the review! To answer the questions:
I think so - I did not update the internal SDK usage as js-client-sdk v4 (at the moment) is still in prerelease. We only import type definitions from
I think the current runtime detection is fine for now as the flag change event payload is an official change between the major versions (it is effectively a slimmed down implementation of type overloading). The version check requires a few changes to the toolbar implementation as the |
….11.0 - Replaced instances of 'launchdarkly-js-client-sdk' with '@launchdarkly/js-client-sdk' across various files. - Updated package.json files in demo and toolbar to include the new SDK version. - Adjusted imports in several components to align with the new SDK structure. - Ensured compatibility with existing code by handling type mismatches where necessary.
- Changed import paths in test files to reflect the new SDK structure. - Updated mock implementations to align with the new SDK methods. - Ensured compatibility with existing tests by adjusting mock behavior.
- Added new compat interfaces so that toolbar could recognize browser v3 and v4 clients. - Changed import paths to use '@launchdarkly/toolbar' and updated related types. > Eventually we should probably provide this compat layer in the browser SDK code itself.
…ovider.tsx Co-authored-by: pranjal-jately-ld <pjately@launchdarkly.com>
a083eb3 to
001edbb
Compare
d387384 to
e5c5389
Compare
Description
This PR will allow the toolbar to be compatible with the new LD js client sdk.
What does this PR do?
Type of Change
Screenshots/Videos