-
Notifications
You must be signed in to change notification settings - Fork 31
chore: add an example app for browser sdk #1019
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
|
@launchdarkly/browser size report |
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
cd49047 to
1048727
Compare
|
@launchdarkly/js-client-sdk size report |
126daf7 to
d701b99
Compare
10632da to
580a21a
Compare
580a21a to
f4ad1e7
Compare
|
NOTE: this example will probably change after we merge #1028 |
| import { initialize } from '@launchdarkly/js-client-sdk'; | ||
|
|
||
| // Set clientSideID to your LaunchDarkly client-side ID | ||
| const clientSideID = ''; |
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.
If we could set this up to work with a .env, or regular environment variable, then that would be good.
Generally speaking this approach results in accidental commits of tokens. Client-side IDs are not secret, so it isn't a big issue, but still nice to not have to worry about it.
If you do use a .env, then you can make sure it is in the git ignore.
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 do the same thing with the flagKey, for convenience.)
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.
yea for this kind of example, I think we can inject those values during build time... I'll refactor this example, once we get the waitForInit PR in.
| render(); | ||
| }); | ||
|
|
||
| ldclient.identify(context).catch(() => { |
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.
Now that we moved to a result returning method, we shouldn't need the catch, correct? Or we need to make sure we cannot throw from that method.
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.
I think we can assume top-level await? Then just await the result?
| render(); | ||
| } else if (status === 'error') { | ||
| div.replaceChild(document.createTextNode('Error identifying client'), div.firstChild as Node); | ||
| } |
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.
Bug: Missing handling for timeout and shed identify statuses
The identify method returns an LDIdentifyResult with four possible status values: completed, error, timeout, and shed. The code only handles completed and error, leaving timeout and shed unhandled. When these occur, the UI remains stuck on "Initializing..." with no user feedback. This is particularly relevant since the browser SDK sets sheddable: true by default, making shed status a realistic scenario.
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.
@joker23 Can we just always render, and when the status is an error additionally show the error box. This would be closer to what we expect.
Also, do we need the try catch?
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.
yea I can get rid of the try catch now that we aren't throwing anything and I'll add a status box to track errors.
d20bdad to
30d81c6
Compare
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.
nit: Newlines at the end of files.
| } | ||
| const flagKey = LD_FLAG_KEY || 'sample-feature'; | ||
| const content = fs.readFileSync(OUTPUT_FILE).toString(); | ||
| fs.writeFileSync(OUTPUT_FILE, content.replaceAll(FLAG_KEY_PLACEHOLDER, flagKey)); |
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.
Bug: Inconsistent fallback behavior for build-time configuration variables
The build configuration has inconsistent handling of environment variables. LD_FLAG_KEY gets a sensible default ('sample-feature') when not provided, but LD_CLIENT_SIDE_ID has no default and leaves the literal placeholder string 'LD_CLIENT_SIDE_ID' in the built output. This causes the SDK to silently fail initialization with an invalid client-side ID if the user forgets to configure it.
c87a4d5 to
4ba5b0b
Compare
This PR will add a new browser SDK example that works with the new 4.x implementation.
I think this also addressed sdk-744
Note
Adds a new browser SDK example app with build/run tooling and includes it in workspaces.
packages/sdk/browser/example):index.html,index.css, andsrc/app.tsdemonstratinginitialize,identify,variation, andchange/errorlisteners with simple UI updates.package.jsonscripts (start,build),tsconfig.json, andtsdown.config.tsthat substitutesLD_CLIENT_SIDE_ID/LD_FLAG_KEYfrom.env.templateintodist/app.js.README.mdwith setup instructions.packages/sdk/browser/exampleto rootpackage.jsonworkspaces.Written by Cursor Bugbot for commit 4ba5b0b. This will update automatically on new commits. Configure here.