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

fix: remove export of OpenFeatureClient #794

Merged
merged 2 commits into from
May 8, 2024

Conversation

lukas-reining
Copy link
Member

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.

@lukas-reining lukas-reining requested a review from a team as a code owner January 27, 2024 23:48
@lukas-reining lukas-reining force-pushed the fix/remove-openfeature-client-export branch 2 times, most recently from 3df41cb to 7efad28 Compare January 28, 2024 00:00
@lukas-reining lukas-reining changed the title fix: remove export of OpenFeatureClient fix!: remove export of OpenFeatureClient Jan 28, 2024
Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

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

Perhaps we should enable the stripInternal compiler option. I haven't used it before but this may be a nice way to ensure we don't accidentally reexport it in the future.

@toddbaert
Copy link
Member

I think we can (debatabley) consider this non-breaking, since it's an export we didn't expect people to use... it really depends how strict we want to be.

In any case, I think it might be a good idea to discuss rolling together some upcoming breaking changes. I'd like to also include:

  • making it impossible to return an error resolution from providers (force them to throw if something goes wrong)
  • removing setProviderAndWait and just making setProvider async

What do you guys think?

cc @lukas-reining @beeme1mr @luizgribeiro @Kavindu-Dodan

@Kavindu-Dodan
Copy link

I think we can (debatabley) consider this non-breaking, since it's an export we didn't expect people to use... it really depends how strict we want to be.

In any case, I think it might be a good idea to discuss rolling together some upcoming breaking changes. I'd like to also include:

  • making it impossible to return an error resolution from providers (force them to throw if something goes wrong)
  • removing setProviderAndWait and just making setProvider async

What do you guys think?

cc @lukas-reining @beeme1mr @luizgribeiro @Kavindu-Dodan

+1 for combining multiple breaking changes for the next release

Copy link
Contributor

@luizgribeiro luizgribeiro left a comment

Choose a reason for hiding this comment

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

I'd say it's a breaking change because if it's shipped as is since it would break minor upgrades in projects that are already using OpenFeatureClient.
I also think bundling more breaking changes in one new major is a good alternative.

Maybe another way that might not be a breaking change is to expose OpenFeatureClient as an alias type to Client.
I'm not a big fan of using Client as a type. Many different packages have a Client exported type and it would end up with an alias anyway.

@toddbaert
Copy link
Member

I'd say it's a breaking change because if it's shipped as is since it would break minor upgrades in projects that are already using OpenFeatureClient. I also think bundling more breaking changes in one new major is a good alternative.

Maybe another way that might not be a breaking change is to expose OpenFeatureClient as an alias type to Client. I'm not a big fan of using Client as a type. Many different packages have a Client exported type and it would end up with an alias anyway.

I think it's OK to require people to alias things, but I would be up for changing the name to something more specific in a 2.0. I definitely don't want to alias the OpenFeatureClient though since it's a class. It's a good idea to export and reference interfaces instead of classes where-ever possible, or you can run into nasty things like this which can happen if people end up with multiple copies of a dep (we do our best to avoid that but it can happen).

@toddbaert
Copy link
Member

I've opened this to record (small) proposed breaking changes in 2.0

@toddbaert toddbaert mentioned this pull request Jan 31, 2024
5 tasks
@toddbaert toddbaert changed the title fix!: remove export of OpenFeatureClient fix: remove export of OpenFeatureClient May 6, 2024
@toddbaert toddbaert force-pushed the fix/remove-openfeature-client-export branch from 4164932 to ff8127c Compare May 6, 2024 19:13
@toddbaert
Copy link
Member

@lukas-reining @luizgribeiro @beeme1mr I've rebased and resolved conflcits on this PR.

I also made the title non-breaking as we decided (though it's technically a break but not on an export we indented people to use).

Signed-off-by: Lukas Reining <lukas.reining@codecentric.de>
Signed-off-by: Lukas Reining <lukas.reining@codecentric.de>
@beeme1mr beeme1mr force-pushed the fix/remove-openfeature-client-export branch from ff8127c to 8ba39b5 Compare May 8, 2024 20:33
@beeme1mr beeme1mr removed the on-hold label May 8, 2024
@beeme1mr beeme1mr added this pull request to the merge queue May 8, 2024
Merged via the queue into main with commit 3d197f2 May 8, 2024
9 checks passed
@beeme1mr beeme1mr deleted the fix/remove-openfeature-client-export branch May 8, 2024 20:42
toddbaert added a commit that referenced this pull request May 13, 2024
🤖 I have created a release *beep* *boop*
---


##
[1.14.0](server-sdk-v1.13.5...server-sdk-v1.14.0)
(2024-05-13)


### 🐛 Bug Fixes

* remove export of OpenFeatureClient
([#794](#794))
([3d197f2](3d197f2))
* removes exports of OpenFeatureClient class and makes event props
readonly ([#918](#918))
([e9a25c2](e9a25c2))
* run error hook when provider returns reason error or error code
([#926](#926))
([c6d0b5d](c6d0b5d))


### 🧹 Chore

* remove node 16
([#875](#875))
([c1878e4](c1878e4))
* **main:** release core 1.2.0
([#927](#927))
([692ad5b](692ad5b))


### 📚 Documentation

* add tip about supported usage in the install section
([#941](#941))
([f0de667](f0de667))

---
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 <109696520+openfeaturebot@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
toddbaert added a commit that referenced this pull request May 13, 2024
🤖 I have created a release *beep* *boop*
---


##
[1.1.0](web-sdk-v1.0.3...web-sdk-v1.1.0)
(2024-05-13)


### ✨ New Features

* set context during provider init on web
([#919](#919))
([7e6c1c6](7e6c1c6))


### 🐛 Bug Fixes

* remove export of OpenFeatureClient
([#794](#794))
([3d197f2](3d197f2))
* removes exports of OpenFeatureClient class and makes event props
readonly ([#918](#918))
([e9a25c2](e9a25c2))
* run error hook when provider returns reason error or error code
([#926](#926))
([c6d0b5d](c6d0b5d))
* skip reconciling event for synchronous onContextChange operations
([#931](#931))
([6c25f29](6c25f29))


### 🧹 Chore

* **main:** release core 1.2.0
([#927](#927))
([692ad5b](692ad5b))


### 📚 Documentation

* add tip about supported usage in the install section
([#941](#941))
([f0de667](f0de667))

---
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 <109696520+openfeaturebot@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
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.

5 participants