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

web-sdk: clientName in EventDetails is overriden #489

Closed
thomaspoignant opened this issue Jul 20, 2023 · 2 comments · Fixed by #490
Closed

web-sdk: clientName in EventDetails is overriden #489

thomaspoignant opened this issue Jul 20, 2023 · 2 comments · Fixed by #490
Assignees

Comments

@thomaspoignant
Copy link
Member

Observed behavior

When writing a provider for the web-sdk, you can emit events and it is possible to provide EventDetails with the event.

const events = new OpenFeatureEventEmitter();
events.emit(ProviderEvents.Error, {
    clientName: 'my-client-name',
    message: 'your message here'
});

When reading the event, from the handler we can see that the value of the client name is not what was provided in the event.

await OpenFeature.setContext(defaultContext);
OpenFeature.setProvider('test-provider', defaultProvider);
const client = await OpenFeature.getClient('test-provider');
client.addHandler(ProviderEvents.Error, (event) => console.log(event));

// Result:
//{
//  clientName: 'test-provider',
//  message: 'Request failed with status code 404'
//}

The value of clientName is the client's name set in setProvider or undefined if we have no name for the provider.

Expected Behavior

Not sure exactly how it should work here, but I see 2 potential solutions:

  1. Don't expose clientName to providers to avoid confusion for people writing providers.
  2. Use the clientName provided in the emit function and forward it in the event.
@beeme1mr
Copy link
Member

Hey @thomaspoignant, I think option 1 makes the most sense. The actual behavior seems to work how I would expect.

I'll take a shot at addressing this.

@beeme1mr beeme1mr self-assigned this Jul 20, 2023
@thomaspoignant
Copy link
Member Author

Yes, I agree for me option 1 is probably the best one.

@beeme1mr beeme1mr linked a pull request Jul 20, 2023 that will close this issue
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 a pull request may close this issue.

2 participants