Skip to content
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

ref(flags): Refactor LaunchDarkly integration to reusable functions #14369

Open
wants to merge 3 commits into
base: aliu/launch-darkly-integration
Choose a base branch
from

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Nov 19, 2024

Moves core logic into re-usable functions so that we can re-use for other integrations.

Depends on #14207

@billyvg billyvg changed the base branch from develop to aliu/launch-darkly-integration November 19, 2024 18:57
@billyvg billyvg changed the title Init package files ref(flags): Refactor LaunchDarkly integration to reusable functions Nov 19, 2024
Copy link
Contributor

github-actions bot commented Nov 19, 2024

size-limit report 📦

Path Size % Change Change
@sentry/browser 22.9 KB added added
@sentry/browser - with treeshaking flags 21.61 KB added added
@sentry/browser (incl. Tracing) 35.45 KB added added
@sentry/browser (incl. Tracing, Replay) 72.14 KB added added
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 62.46 KB added added
@sentry/browser (incl. Tracing, Replay with Canvas) 76.44 KB added added
@sentry/browser (incl. Tracing, Replay, Feedback) 88.92 KB added added
@sentry/browser (incl. Feedback) 39.64 KB added added
@sentry/browser (incl. sendFeedback) 27.54 KB added added
@sentry/browser (incl. FeedbackAsync) 32.34 KB added added
@sentry/react 25.6 KB added added
@sentry/react (incl. Tracing) 38.32 KB added added
@sentry/vue 27.07 KB added added
@sentry/vue (incl. Tracing) 37.27 KB added added
@sentry/svelte 23.05 KB added added
CDN Bundle 24.08 KB added added
CDN Bundle (incl. Tracing) 37.04 KB added added
CDN Bundle (incl. Tracing, Replay) 71.74 KB added added
CDN Bundle (incl. Tracing, Replay, Feedback) 77.09 KB added added
CDN Bundle - uncompressed 71.03 KB added added
CDN Bundle (incl. Tracing) - uncompressed 110.25 KB added added
CDN Bundle (incl. Tracing, Replay) - uncompressed 222.78 KB added added
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 236 KB added added
@sentry/nextjs (client) 38.43 KB added added
@sentry/sveltekit (client) 35.96 KB added added
@sentry/node 134.49 KB added added
@sentry/node - without tracing 96.33 KB added added
@sentry/aws-serverless 106.57 KB added added

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine we'll want to use this in node/server environments as well, would it make sense to move these to core?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the handler being used in the LD integration is only available for client-side js, that's why this was moved to the browser. Not sure if this will change for other integrations in the future

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing in these util functions are specific to LD integration, it stores flag values to scope. It's possible for a server side implementation to re-use these functions.

@billyvg billyvg marked this pull request as ready for review November 19, 2024 22:09
Copy link
Member

@chargome chargome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

dev-packages/browser-integration-tests/utils/helpers.ts Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the handler being used in the LD integration is only available for client-side js, that's why this was moved to the browser. Not sure if this will change for other integrations in the future

@billyvg billyvg requested review from a team as code owners November 20, 2024 16:57
@billyvg billyvg force-pushed the ref-ff-launch-darkly-integration-abstraction branch from a094ad5 to d124fef Compare November 20, 2024 16:58
@billyvg billyvg force-pushed the ref-ff-launch-darkly-integration-abstraction branch from 39583db to 4195a8e Compare November 20, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants