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

Ability to set configuration request parameters on an already instantiated client instance #37

Merged
merged 2 commits into from
Feb 3, 2024

Conversation

aarsilv
Copy link
Contributor

@aarsilv aarsilv commented Feb 3, 2024

🎟️ Ticket: (Eppo Internal) FF-1557 - JS Client SDK should return null when not initialized
πŸ‘― Related PR: eppo-client-sdk #48

Motivation and Context

Using EppoRandomizationProvider without waitForInitialization = true or any other JavaScript pattern where initialization of the client is not awaited, could result in some getInstance() and getAssignment() calls on an uninitialized instance.

Previous changes to js-client-sdk made it so that it threw an error (link). However, we want to revert to the behavior of returning null, which this PR does (but now with a warning) on calls to getAssignment().

To accomplish this, we need to revert to having a default initialized instance (link to where that was changed) that is later populated during initialization.

Description

To allow upstream-created client instances to set configuration request parameters, we provide a new method: setConfigurationRequestParameters().

I also renamed the internal variables to configurationRequestParameters to more consistent and not have "config" in them twice.

How has this been tested?

  • New test added to eppo-client.spec.ts
  • This module was locally installed in the js-client-sdk and node-server-sdk repositories using make prepare and yarn add --force --file:../js-client-sdk-common

Comment on lines +1183 to +1185
await client.fetchFlagConfigurations();
variation = client.getAssignment(subjectForGreenVariation, flagKey);
expect(variation).toBe('green');
Copy link
Member

Choose a reason for hiding this comment

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

very nice

Copy link
Contributor

@sameerank sameerank left a comment

Choose a reason for hiding this comment

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

πŸ‘

throw new Error(
'Eppo SDK unable to fetch flag configurations without a request configuration',
'Eppo SDK unable to fetch flag configurations without a configuration request parameters',
Copy link
Contributor

Choose a reason for hiding this comment

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

Parameters is plural so "Eppo SDK unable to fetch flag configurations without a configuration request parameters"

@aarsilv aarsilv assigned aarsilv and unassigned sameerank Feb 3, 2024
@aarsilv aarsilv merged commit b04f796 into main Feb 3, 2024
2 checks passed
@aarsilv aarsilv deleted the aaron/ff-1557/set-request-configuration-later branch February 3, 2024 00:50
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.

3 participants