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

docs: Nest SDK #750

Merged
merged 25 commits into from
Jan 24, 2024
Merged

docs: Nest SDK #750

merged 25 commits into from
Jan 24, 2024

Conversation

luizgribeiro
Copy link
Contributor

This PR

  • Adds docs to the NestJS SDK.

Notes

I think we're probably reaching a stable point for the NestJS SDK.
It would be great to have some feedback on the docs before it.

Follow-up Tasks

  • Blog post & announcement regarding the new SDK (maybe?)

Signed-off-by: Luiz Ribeiro <[email protected]>
Signed-off-by: Luiz Ribeiro <[email protected]>
@luizgribeiro luizgribeiro requested a review from a team as a code owner January 10, 2024 01:07
@luizgribeiro luizgribeiro changed the title Docs/next sdk docs: Next SDK Jan 10, 2024
packages/nest/README.md Outdated Show resolved Hide resolved
packages/nest/README.md Outdated Show resolved Hide resolved
packages/nest/README.md Outdated Show resolved Hide resolved
packages/nest/README.md Outdated Show resolved Hide resolved
packages/nest/src/open-feature.module.ts Outdated Show resolved Hide resolved
@weyert weyert changed the title docs: Next SDK docs: Nest SDK Jan 10, 2024
luizgribeiro and others added 3 commits January 10, 2024 09:32
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
packages/nest/README.md Outdated Show resolved Hide resolved
packages/nest/README.md Outdated Show resolved Hide resolved
packages/nest/README.md Outdated Show resolved Hide resolved
packages/nest/README.md Outdated Show resolved Hide resolved
packages/nest/README.md Outdated Show resolved Hide resolved
packages/nest/README.md Show resolved Hide resolved
packages/nest/src/evaluation-context-interceptor.ts Outdated Show resolved Hide resolved
packages/nest/src/open-feature.module.ts Outdated Show resolved Hide resolved
Comment on lines 131 to +132
@FeatureClient() private defaultClient: Client,
@FeatureClient({ name: 'differentServer' }) private namedClient: Client,
) {
}
@FeatureClient({ name: 'differentProvider' }) private namedClient: Client,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should rename this to OpenFeatureClient before we remove the experimental. I think just FeatureClient is a bit disjointed, since we have tended to either add the entire "OpenFeature" prefix, or leave it off. I think OpenFeatureClient would be best, because the client inteface is called Client so it won't clash, and it's consistent with the equivalent in react (</OpenFeatureProvider>).

Copy link
Member

@lukas-reining lukas-reining Jan 14, 2024

Choose a reason for hiding this comment

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

It has been OpenFeatureClient before, but OpenFeatureClient is used for the implementation of the Client and it is also exported from the Server SDK.

We could remove the export of OpenFeatureClient from the Web SDK, as no one should use and import this directly anyways I would say. I have seen it several times that this type was used instead of Client in projects.
If we remove this I would be happy calling the interface OpenFeatureClienthere. What do you think @toddbaert?

Copy link
Member

Choose a reason for hiding this comment

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

What do you think @toddbaert?

Copy link
Member

@toddbaert toddbaert Jan 19, 2024

Choose a reason for hiding this comment

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

Sorry for the late reply - if we can remove that export, I'm fine with it! It's an improvement.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @toddbaert, removing this should be considered a breaking change I would say.
I am not sure how we want to continue on this then. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

hmm, well, if I understand correctly, it would be technically breaking, but I think it could be argued it was a misuse. All our doc recommends using the interface.

If we want to be really strict, we could consider it a break and include it with some others I have in mind in a 2.0.0 (meaning to ask you guys about this soon... there's a small list of breaking improvements I think might be worth considering)...

packages/nest/README.md Outdated Show resolved Hide resolved
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

I think you should add a release version tag like this example for auto-inclusion of this SDK in docs.

You can also add a features table like this. The odd situation with this SDK and the React SDK (which I'm working on) is that they get some features "inhereted" from the associated parent SDK - so we may want to indicate that with some new symbol in the table. Any idas? @luizgribeiro @lukas-reining ?

We also don't have to settle everything in this PR. I will happily come back and add these little enhancements for doc inclusions later.

We could also "hide" the features tables entirely for these "integration-level" SDKs, since they have unique feature-sets related to their framework... we could consider some SDKs "language-sdks", and some "framework-sdks" and they could behave differently in the docs. cc @beeme1mr

luizgribeiro and others added 3 commits January 15, 2024 21:34
Co-authored-by: Lukas Reining <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
Co-authored-by: Lukas Reining <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
Co-authored-by: Lukas Reining <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
@luizgribeiro
Copy link
Contributor Author

We could also "hide" the features tables entirely for these "integration-level" SDKs, since they have unique feature-sets related to their framework... we could consider some SDKs "language-sdks", and some "framework-sdks" and they could behave differently in the docs. cc

@toddbaert, what do you think about using something like a SDK label to make the difference between language/runtime level SDKs and framework level SDKs? We're facing this in JS right now, but will probably be a valid point in other languages and frameworks as well

Signed-off-by: Luiz Ribeiro <[email protected]>
@lukas-reining
Copy link
Member

You can also add a features table like this. The odd situation with this SDK and the React SDK (which I'm working on) is that they get some features "inhereted" from the associated parent SDK - so we may want to indicate that with some new symbol in the table. Any idas? @luizgribeiro @lukas-reining ?

@toddbaert yes, maybe we could give this a Framework integration semantic. For the feature tables, I think we could maybe leave it out, as the (at least my) assumption is that the base SDK will always provide the feature set that is salwys available and these Integration SDKs are just gonna make life better and maybe even add additional specific features. What do you think?

@lukas-reining
Copy link
Member

I think you should add a release version tag like this example for auto-inclusion of this SDK in docs.

This is good, @luizgribeiro could you add this?

Signed-off-by: Luiz Ribeiro <[email protected]>
luizgribeiro and others added 7 commits January 22, 2024 21:03
Co-authored-by: Lukas Reining <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
Co-authored-by: Lukas Reining <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
Co-authored-by: Lukas Reining <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
Signed-off-by: Luiz Ribeiro <[email protected]>
@luizgribeiro
Copy link
Contributor Author

I've tried to come up with a nice feature table but it ended up pretty repetitive from what we already have from the server-sdk, so I decided to remove it.

In this framework integration we could use something like a "compatibility table" instead of a feature table.
By doing this it would be possible to express which features from the OF base SDK are already covered and which are not or wip.

@beeme1mr
Copy link
Member

@toddbaert and I were talking about something similar for the react SDK. We'll just need to come up with a pattern for these framework specific SDKs.

packages/nest/README.md Outdated Show resolved Hide resolved
packages/nest/README.md Outdated Show resolved Hide resolved
packages/nest/README.md Outdated Show resolved Hide resolved
@beeme1mr
Copy link
Member

Awesome job @luizgribeiro and @lukas-reining.

luizgribeiro and others added 3 commits January 23, 2024 21:58
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Luiz Guilherme Ribeiro <[email protected]>
Signed-off-by: Lukas Reining <[email protected]>
Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

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

This looks really good now @luizgribeiro!
I will merge it and we can refine more details later if needed.

@lukas-reining lukas-reining added this pull request to the merge queue Jan 24, 2024
Merged via the queue into open-feature:main with commit a21634d Jan 24, 2024
7 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Jan 31, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.0.5-experimental](nestjs-sdk-v0.0.4-experimental...nestjs-sdk-v0.0.5-experimental)
(2024-01-31)


### ✨ New Features

* adds ErrorOptions to Error constructor
([#765](#765))
([2f59a9f](2f59a9f))


### 🐛 Bug Fixes

* **nest:** add peer deps for nestjs-core and rxjs 9
([#784](#784))
([a452bdd](a452bdd))
* removed duped core types
([#800](#800))
([7cc1e09](7cc1e09))


### 📚 Documentation

* Nest SDK ([#750](#750))
([a21634d](a21634d))
* update nestjs readme examples
([#787](#787))
([c6a357a](c6a357a))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Signed-off-by: OpenFeature Bot <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request May 8, 2024
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
As discussed here
#750 (comment),
we should not export `OpenFeatureClient` from the server and web SDK.
The type used outside the SDK should be `Client`, which is also used in
the public APIs like `OpenFeatureApi`.

The question is, if we should mark this as breaking. 
Technically it will break code that imports `OpenFeatureClient` instead
of `Client`, but as @toddbaert said code using it could also be seen as
"used wrong" while being technically fine.

I am still leaning towards marking it as breaking, to be sure we are not
breaking something that is technically fine, just because it is
unintended use. But I could also live with non-beaking.

---------

Signed-off-by: Lukas Reining <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request May 19, 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 -->

As discussed here
[#750](#750 (comment)),
renames `@FeatureClient` to `@OpenFeatureClient` which is possible since
we merged #918.

Signed-off-by: Lukas Reining <[email protected]>
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.

4 participants