Skip to content

chore: experimental FDv2 configuration hooked up #853

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

Open
wants to merge 1 commit into
base: ta/fdv2-code-move-part3
Choose a base branch
from

Conversation

tanderson-ld
Copy link
Contributor

@tanderson-ld tanderson-ld commented May 14, 2025

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

This PR adds new changes to LdClientImpl to utilize FDv2 as an experimental feature.
This PR also moves changes for FDv2 from the temporary holding branch to main.

@tanderson-ld tanderson-ld changed the title Ta/sdk 857/fdv2 config to main chore: experimental FDv2 configuration hooked up May 14, 2025
Copy link
Contributor

@launchdarkly/js-sdk-common size report
This is the brotli compressed size of the ESM build.
Size: 24210 bytes
Size limit: 25000

Copy link
Contributor

@launchdarkly/js-client-sdk size report
This is the brotli compressed size of the ESM build.
Size: 20738 bytes
Size limit: 21000

Copy link
Contributor

@launchdarkly/js-client-sdk-common size report
This is the brotli compressed size of the ESM build.
Size: 15786 bytes
Size limit: 20000

@tanderson-ld tanderson-ld changed the base branch from main to ta/fdv2-code-move-part3 May 14, 2025 19:22
if (this._initPhaseActive && this._initFactories.pos() >= this._initFactories.length()) {
this._initPhaseActive = false;
this._syncFactories.reset();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: Bugfix found during testing with intermittent wifi. Error in last initializer was not leading to fallback to synchronizer.

@@ -93,17 +105,19 @@ const VARIATION_METHOD_DETAIL_NAME = 'LDClient.variationDetail';
export default class LDClientImpl implements LDClient {
private _initState: InitState = InitState.Initializing;

private _featureStore: LDFeatureStore;
private _featureStore!: LDFeatureStore | LDTransactionalFeatureStore;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: The !s added to various members is due to the usage of initializing functions inside the constructor and TS linter not recognizing all paths set those values. Alan and I tried use local function to work around it, but that didn't work either. Perhaps @kinyoklion , you have some ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For Reviewers: These changes have never been reviewed before, please review thoroughly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: These changes have been reviewed before EXCEPT for "experimental commenting".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: These changes have been reviewed before EXCEPT it has been modified to not adapt FDv1 configuration to the FDv2 data system.

dsErrors.forEach((error) => {
this.logger?.warn(error);
});
}
Copy link
Contributor Author

@tanderson-ld tanderson-ld May 14, 2025

Choose a reason for hiding this comment

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

For reviewers: When this code was previously reviewed when merging to the FDv2 holding branch, there was an else block here that would map the FDv1 options to an FDv2 dataSystem. That has been removed to have FDv1 and FDv2 not have code overlap/separability.

@tanderson-ld tanderson-ld marked this pull request as ready for review May 14, 2025 19:31
@tanderson-ld tanderson-ld requested a review from a team as a code owner May 14, 2025 19:31
@tanderson-ld
Copy link
Contributor Author

Do not merge into target branch until target merges to main.

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.

1 participant