-
Notifications
You must be signed in to change notification settings - Fork 3
refactor highlight.run SDK into plugins #21
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
Conversation
8299474 to
d815094
Compare
|
Things should work with launchdarkly-js-client-sdk 3.x as well, correct? Or did you find something lacking in the interfaces? |
tested with 3.x: helped find a bug because the 0.6.0 @launchdarkly/js-client-sdk sdk version didn't have the |
ccschmitz-launchdarkly
left a comment
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.
Very cool! Left a couple thoughts, but will leave the real review to @kinyoklion.
I mainly looked over the observe plugin. I'm sure we don't want to diverge too much from the existing implementation, but was wondering if we wanted to take the opportunity to clean up some of the rough edges, mainly deprecated or poorly chosen config options.
| [FEATURE_FLAG_CLIENT_SIDE_ID_ATTR]: metadata.clientSideId, | ||
| [FEATURE_FLAG_ENV_ATTR]: metadata.application?.id, |
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.
@Vadman97 Please forgive the comment without context - is application.id the right thing to pass here? I'm not sure if this is just a terminology thing (in LD terms "application" is a separate concept from "environment", but I'm not sure about the Highlight terminology), or if we've arranged for the plugins to pass the LD environment id in the application id field, etc.
Also, fun fact, the LD client-side id is actually identical to the LD environment id, so we might not need to pass both. (The client-side id is also used as a credential - albeit a weird one that's not treated as secret - so it might be better not to include it per se in the telemetry unless we have a good use case for it?)
Summary
Refactors the
highlight.runSDK into plugins that can be connected to the3.xand4.xLaunchDarkly SDKs.How did you test this change?
local testing with react-router app, new unit tests
validating bundling working as expected (


@launchdarkly/observabilitydoes not bundle rrweb,@launchdarkly/session-replaybundles observability features since it uses them and rrweb)Are there any deployment considerations?
0.x versions of
@launchdarklypackages released.changeset for
highlight.runDoes this work require review from our design team?
no