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

Nest.js SDK #705

Open
4 tasks
lukas-reining opened this issue Dec 8, 2023 · 17 comments · Fixed by #718
Open
4 tasks

Nest.js SDK #705

lukas-reining opened this issue Dec 8, 2023 · 17 comments · Fixed by #718
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed sdk

Comments

@lukas-reining
Copy link
Member

lukas-reining commented Dec 8, 2023

We should build a Nest.js Module, as Nest.js is a really popular backend framework for JavaScript.

The following is the current idea of what feature could be interesting:

  • Dynamic module with forRoot receiving a list of OpenFeature clients
  • A decorator to inject a client (named and unnamed)
  • A decorator to inject a feature flag into a method (if good to implement)
  • It should be possible to easily set the context of the feature flag evaluation in the scope of a request

Non-functional requirements:

  • consistent terminology and naming with Web SDK and OpenFeature spec
  • observe Nest.js idioms and patterns
  • accurate peer dependency for the Web SDK
@luizgribeiro
Copy link
Contributor

luizgribeiro commented Dec 8, 2023

That sounds good!
I have some questions though.

Dynamic module with forRoot receiving a list of OpenFeature clients
Probably providers, no? I think that something like the following structure would do the job:

{
     defaultProvider: Provider();
     namedProviders: [
       {
          name: "Some provider",
          provider: Provider()
       }
     ]
}

This way we can keep it simple for those who only want a default provider but also support different scenarios.

A decorator to inject a feature flag into a method
You mean a parameter decorator? Maybe one limitation is the fact that flag evaluation gives a Promise. I'm not shure if this will work

I'd also say that a Method/Controller/Handler decorator can be useful. Enabling or disabling an enpoint from a decorator might be useful

@beeme1mr
Copy link
Member

beeme1mr commented Dec 8, 2023

It would also be great if it were possible to configure the injected client to contain request-scoped context properties. That would allow you to easily set context like targetingKey, email, or ip that would apply to all flag evaluations made by the client.

By the way, the OpenFeature playground app uses Nest. We should keep that in mind as we build out this SDK.

@lukas-reining
Copy link
Member Author

It would also be great if it were possible to configure the injected client to contain request-scoped context properties. That would allow you to easily set context like targetingKey, email, or ip that would apply to all flag evaluations made by the client.

Ya totally, changed the requirements.

@lukas-reining lukas-reining added help wanted Extra attention is needed enhancement New feature or request sdk labels Dec 8, 2023
@lukas-reining
Copy link
Member Author

lukas-reining commented Dec 8, 2023

Probably providers, no? I think that something like the following structure would do the job:

Yes this could make sense.
But I would be fine with any implementation that fits the provider/client implementation that we have right now.

@lukas-reining
Copy link
Member Author

lukas-reining commented Dec 8, 2023

I'd also say that a Method/Controller/Handler decorator can be useful. Enabling or disabling an enpoint from a decorator might be useful.

This sounds good!
The question here will be if the ergonomics of this feature would be good and looking what disabling of an endpoint means.

@lukas-reining
Copy link
Member Author

I will start to bring some of the existing code into a draft PR and then we can continue to work on this.

@luizgribeiro
Copy link
Contributor

The question here will be if the ergonomics of this feature would be good and looking what disabling of an endpoint means.

I think that returning 404 in this case would mean that it's disabled.

I see that we're coming up with multiple ideas and it will probably make it harder to implement all at once.
What do you think about defining deliverable scopes for this and make it incremental?

@luizgribeiro
Copy link
Contributor

A decorator to inject a feature flag into a method (if good to implement)

Do you have any idea on how to implement this?
I did a few tests with async param decorators but didn't have any success 😞

@lukas-reining
Copy link
Member Author

Do you have any idea on how to implement this?
I did a few tests with async param decorators but didn't have any success 😞

Yes but I am not sure about the ergonomics.
The following works, and instead of returning a Promise<EvaluationDetails> we could also wrap it in an observable.
In the end we will have to decide if it is "usable" enough.

export interface OpenFeatureStringFlagProps {
  clientName?: string;
  flagKey: string;
  defaultValue: string;
  context?: EvaluationContext;
}

export const OpenFeatureStringFlag= createParamDecorator(
  ({ clientName, flagKey, defaultValue, context }: OpenFeatureFeatureProps) => {
    const client = clientName ? OpenFeature.getClient(clientName) : OpenFeature.getClient();
    return client.getStringDetails(flagKey, defaultValue, context);
  },
);
import { Controller, Get } from '@nestjs/common';
import { EvaluationDetails } from '@openfeature/server-sdk';
import { OpenFeatureFeature } from './feature.decorator';

@Controller()
export class ExampleController {
  @Get('/tenant')
  public async handleRequest(
    @OpenFeatureStringFlag({ clientName: 'myCLient', flagKey: 'tenantName', defaultValue: 'BigCorp' })
    feature: Promise<EvaluationDetails<string>>,
  ) {
    const details = await feature;
    return details.value;
  }
}

@lukas-reining
Copy link
Member Author

I think that returning 404 in this case would mean that it's disabled.

Yes, it would only be important, that this behaves exactly like the endpoint not existing, I would say.

@luizgribeiro
Copy link
Contributor

I've started sketching a module here but I'm not 100% confident about this. I'll probably study libs such as nest/mongoose before coming up with anything barely usable. I haven't done much Dynamic modules stuff from ground. My main concern about this is how to test it properly.

@lukas-reining
Copy link
Member Author

lukas-reining commented Dec 12, 2023

I've started sketching a module here but I'm not 100% confident about this. I'll probably study libs such as nest/mongoose before coming up with anything barely usable. I haven't done much Dynamic modules stuff from ground. My main concern about this is how to test it properly.

As promised, I started to integrate this too. Here is the first draft of the most basic functions: #718
I chose a testing strategy, maybe you can give some feedback.
From this PR we could start and add features to the SDK, or change the API completely.
Would be happy to do this together @luizgribeiro.

@luizgribeiro I also wanted to add you as reviewer, but you are not in the org. Would you like to joint the org? Then we could add you.

github-merge-queue bot pushed a commit that referenced this issue Dec 13, 2023
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
<!-- add the description of the PR here -->

This is a first draft of a Nest.js SDK.
Currently it only handles injecting clients and feature flags.
I also added a context factory for the FeatureFlag decorators to be able
to use request information for the execution context.

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

Fixes #705 

### Notes
<!-- any additional notes for this PR -->

@toddbaert maybe we can do it as we did with the React SDK, merge this
fast and release as experimental to let people experiment and try out?

### Follow-up Tasks
<!-- anything that is related to this PR but not done here should be
noted under this section -->
<!-- if there is a need for a new issue, please link it here -->
We will have to see if we want to stick with the injected feature flags
being wrapped in an observable.
To me this feels the most idiomatic for Nest.js.

### How to test
Tests for the current functionality are included.

---------

Signed-off-by: Lukas Reining <[email protected]>
Co-authored-by: Henry Chen <[email protected]>
Co-authored-by: Luiz Guilherme Ribeiro <[email protected]>
Co-authored-by: Todd Baert <[email protected]>
@lukas-reining lukas-reining reopened this Dec 13, 2023
@lukas-reining
Copy link
Member Author

We are discussing context propagation here: #736

toddbaert added a commit that referenced this issue Jan 11, 2024
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
<!-- add the description of the PR here -->

This is a first draft of a Nest.js SDK.
Currently it only handles injecting clients and feature flags.
I also added a context factory for the FeatureFlag decorators to be able
to use request information for the execution context.

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

Fixes #705 

### Notes
<!-- any additional notes for this PR -->

@toddbaert maybe we can do it as we did with the React SDK, merge this
fast and release as experimental to let people experiment and try out?

### Follow-up Tasks
<!-- anything that is related to this PR but not done here should be
noted under this section -->
<!-- if there is a need for a new issue, please link it here -->
We will have to see if we want to stick with the injected feature flags
being wrapped in an observable.
To me this feels the most idiomatic for Nest.js.

### How to test
Tests for the current functionality are included.

---------

Signed-off-by: Lukas Reining <[email protected]>
Co-authored-by: Henry Chen <[email protected]>
Co-authored-by: Luiz Guilherme Ribeiro <[email protected]>
Co-authored-by: Todd Baert <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Jan 18, 2024
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
<!-- add the description of the PR here -->

Adds options for logger, event handlers and hooks to Nest SDK
initialization.
This change makes the SDK completely configurable during the
initialization without the need to explicitly call the OpenFeature API.

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

Relates to #705

---------

Signed-off-by: Lukas Reining <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
Co-authored-by: Luiz Guilherme Ribeiro <[email protected]>
@beeme1mr
Copy link
Member

beeme1mr commented Sep 3, 2024

Hey @lukas-reining, can this issue be closed?

@lukas-reining
Copy link
Member Author

Good question, maybe we can take the chance to see if we can get to a 1.0 release.
What do you think @beeme1mr?

@beeme1mr
Copy link
Member

beeme1mr commented Sep 3, 2024

Sure, that sounds reasonable.

@lukas-reining
Copy link
Member Author

Sure, that sounds reasonable.

Will propose it in the community meeting and then we can do it maybe. @beeme1mr @toddbaert

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed sdk
Projects
Status: Preview
Development

Successfully merging a pull request may close this issue.

3 participants