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

feat: add initial SDK library boilerplate and basic svelte LD SDK #632

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nosnibor89
Copy link

@nosnibor89 nosnibor89 commented Oct 23, 2024

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines (TBH yarn run contract-tests failed for me even in main)
  • I have validated my changes against all supported platform versions

Related issues

No issue

Describe the solution you've provided

Introducing the new @launchdarkly/svelte-client-sdk package. Some of the details included in this PR are

  1. Svelte Library Boilerplate
  2. Basic Svelte SDK functionality:
    2.1 LDProvider component
    2.2 LDFlag component
    2.3 Svelte-compatible LD instance (exposes API to work with feature flags)

Describe alternatives you've considered

I don't know what to write here.

Additional context

This is the first of a series of PRs. Some of the following PR should be about

  1. Adding Documentation for @launchdarkly/svelte-client-sdk
  2. Adding Example project that uses @launchdarkly/svelte-client-sdk

@nosnibor89 nosnibor89 changed the title add initial SDK library boilerplate and basic svelte LD SDK feat: add initial SDK library boilerplate and basic svelte LD SDK Oct 23, 2024
@nosnibor89 nosnibor89 marked this pull request as ready for review October 23, 2024 21:50
@nosnibor89 nosnibor89 requested a review from a team as a code owner October 23, 2024 21:50
@kinyoklion
Copy link
Member

Hello @nosnibor89,

Thank you for the contribution.

Those contract tests aren't relevant to client-side SDKs, but they should all work correctly (and do run in CI and passed for your PR). When developing locally running yarn build in the root of the repo should get everything into a functioning state. (Builds all packages, and everything in the workspace will use local packages.) The contract tests do run a go binary downloaded from github, so that could cause problems for some configurations.

I won't be able to review this deeply this week, but hopefully I can next week.

Thank you,
Ryan

@@ -0,0 +1,244 @@
import * as LDClient from 'launchdarkly-js-client-sdk';
Copy link
Member

Choose a reason for hiding this comment

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

We will be releasing the new SDK soon, which comes from this repo and is in the 'browser' directory. It will be the @launchdarkly/js-client-sdk.

The interface is very similar though, so changes should be minimal.

Choose a reason for hiding this comment

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

Nice! I'll take peak next week and see how I could use it. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

@launchdarkly/js-client-sdk If you use the same version number as is in the repo it will use the version from the workspace.

@launchdarkly/js-client-sdk/compat provide a compatibility interface to be mostly compatible with the old package.

".": {
"types": "./dist/index.d.ts",
"svelte": "./dist/index.js",
"default": "./dist/index.js"
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like svelte doesn't support CJS? That makes things very pleasant.

"test:unit-ui": "vitest --ui"
},
"peerDependencies": {
"@launchdarkly/js-client-sdk-common": "^1.1.4",
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting mix.

launchdarkly-js-client-sdk doesn't use @launchdarkly/js-client-sdk-common

But the new one does, but theoretically we would only want to use one or the other.

LDClient,
LDFlagSet,
LDFlagValue,
LDContext as NodeLDContext,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe some non-node name. BrowserLDContext?

* @param {string} flagKey - The key of the flag to watch.
* @returns {Readable<LDFlagsValue>} A readable store of the flag value.
*/
const watch = (flagKey: string): Readable<LDFlagsValue> =>
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure how it needs to be handled with Svelte, but when a flag is accessed we have to make sure an event is sent to launchdarkly. This is accomplished with a variation/variation detail call.

Part of the SDKs job is providing access to flags, but the other part is driving product features.

Flag insights and experimentation, for instance, require those events to function.

For react we use a Proxy: https://github.com/launchdarkly/react-client-sdk/blob/21c2402f6d591caf61764ba35263ff1e4cb18ae7/src/getFlagsProxy.ts#L61

Ideally though we would listen to specific flags.

* @param {string} flagKey - The key of the flag to check.
* @returns {boolean} True if the flag is on, false otherwise.
*/
const isOn = (flagKey: string): boolean => {
Copy link
Member

Choose a reason for hiding this comment

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

So, we could have something like this that was useValue, but we wouldn't want a pure boolean result.

Ideally each flag access takes a default, and that default goes to variation, and then that can be included in events to drive product features. So ideally we would focus the interface on individual flag access.

On means something different though. Lets imagine a flag called backgroundColor, when it is on it serves 5% of users blue, and the rest yellow, but when it is off it serves white.

On/Off is part of the reason for an evaluation, but not derived from its value.

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.

4 participants