-
Notifications
You must be signed in to change notification settings - Fork 31
refactor(sdk-client): replace ConfigurationImpl class with createConfiguration factory function #1068
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
…iguration factory function SDK-1705: Refactor ConfigurationImpl to follow factory function pattern - Add createConfiguration() factory function that returns Configuration interface - Update LDClientImpl to use createConfiguration() instead of new ConfigurationImpl() - Update exports in configuration/index.ts to include createConfiguration - Keep ConfigurationImpl class for backwards compatibility - Update test files to use createConfiguration() - Update tests to check serviceEndpoints properties instead of internal class properties This change follows the CONTRIBUTING.md guidelines for preferring interfaces over classes, which results in better bundle size and minification potential. Co-Authored-By: Steven Zhang <zhangksteven@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/browser size report |
SDK-1705: Complete removal of ConfigurationImpl class - Remove ConfigurationImpl class from Configuration.ts - Update exports in index.ts to remove ConfigurationImpl - All usages now use createConfiguration() factory function This completes the refactoring to follow the CONTRIBUTING.md guidelines for preferring interfaces over classes. Co-Authored-By: Steven Zhang <zhangksteven@gmail.com>
54c0dde to
efe9fac
Compare
Use object spread instead of explicit property mapping in the return statement to reduce the generated JavaScript code size. Before optimization: +716 bytes raw, +86 bytes brotli After optimization: -319 bytes raw, -35 bytes brotli (vs main) Co-Authored-By: Steven Zhang <zhangksteven@gmail.com>
Co-Authored-By: Steven Zhang <zhangksteven@gmail.com>
Removes the separate validateTypesAndNames function and inlines the validation logic directly into createConfiguration since it's only used in one place. This further reduces the bundle size. Additional savings: ~443 bytes raw, ~54 bytes brotli Co-Authored-By: Steven Zhang <zhangksteven@gmail.com>
Requirements
Related issues
SDK-1705
Describe the solution you've provided
This PR replaces the
ConfigurationImplclass with acreateConfiguration()factory function following the CONTRIBUTING.md guidelines for preferring interfaces over classes. The factory function returns aConfigurationinterface object instead of instantiating a class, which results in better bundle size and minification potential.Key changes:
ConfigurationImplclass withcreateConfiguration()factory function inConfiguration.tsLDClientImplto usecreateConfiguration()instead ofnew ConfigurationImpl()configuration/index.tsto exportcreateConfiguration(removedConfigurationImpl)createConfiguration()and verify properties through the publicConfigurationinterfaceImportant for reviewers:
baseUri,eventsUri,streamUriwere changed to checkserviceEndpoints.polling,serviceEndpoints.streaming,serviceEndpoints.eventsrespectively - these internal URI properties are not part of the publicConfigurationinterfacepayloadFilterKeytests now checkserviceEndpoints.payloadFilterKeysince that's where the value is stored in the public interfaceConfigurationImplwas removed entirely - this is a breaking change if any external consumers were using it directly (internal usage only expected)Bundle size impact:
Human review checklist:
ConfigurationImplexportbaseUri,eventsUri,streamUri,payloadFilterKeyare destructured out)Describe alternatives you've considered
Could have kept
ConfigurationImplfor backwards compatibility, but since it's internal-only usage, removing it entirely provides cleaner code and better bundle size.Additional context
Link to Devin run: https://app.devin.ai/sessions/608a317f9f334f38ab018c4b83ed3b4c
Requested by: Steven Zhang (@joker23)
Note
Medium Risk
Touches core config construction/validation and exported API surface; behavior should be equivalent but any subtle differences in option coercion or unknown-option handling could affect SDK initialization.
Overview
Refactors SDK configuration creation by removing the
ConfigurationImplclass and introducing acreateConfiguration()factory that builds a plainConfigurationobject, inlining option validation/coercion and constructingServiceEndpointswhile omitting internal URI fields from the returned shape.Updates
LDClientImpland unit tests to usecreateConfiguration(), and adjusts assertions to validate endpoint defaults andpayloadFilterKeyviaconfig.serviceEndpointsrather than internalbaseUri/streamUri/eventsUriproperties; updatesconfiguration/index.tsexports accordingly.Written by Cursor Bugbot for commit f194310. This will update automatically on new commits. Configure here.