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.

Sign up for GitHub

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

Jump to bottom

feat: add unleash web provider #1105

Merged
merged 20 commits into from
Jan 10, 2025
Merged

feat: add unleash web provider #1105

merged 20 commits into from
Jan 10, 2025

Conversation

jarebudev
Copy link
Contributor

This PR

  • adds an unleash provider

Related Issues

Fixes #1069

Notes

Follow-up Tasks

  • Missing unit tests to verify that the context is updated - will add these

How to test

  • There are Jest unit tests

@jarebudev jarebudev requested a review from a team as a code owner December 5, 2024 23:32
@beeme1mr beeme1mr self-assigned this Dec 9, 2024
@lukas-reining lukas-reining self-assigned this Dec 13, 2024
@lukas-reining
Copy link
Member

Hey @jarebudev I will have a look at this in the next days. Sorry for the delay :)

@jarebudev
Copy link
Contributor Author

Hey @jarebudev I will have a look at this in the next days. Sorry for the delay :)

No worries :)

@toddbaert toddbaert changed the title feat: add unleash provider feat: add unleash web provider Dec 17, 2024
lukas-reining
lukas-reining reviewed Dec 18, 2024
View reviewed changes
Copy link
Member

@lukas-reining lukas-reining left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the delay again, I had quite busy days.
This looks really good! :)
I added a comment regarding the type checking and conversion, please have a look if you agree.
Tomorrow I will have another look but this already looks pretty good.

Edit: I will have a look at the failing tests tomorrow too.

libs/providers/unleash-web/src/lib/unleash-web-provider.ts Outdated Show resolved Hide resolved
libs/providers/unleash-web/src/lib/unleash-web-provider.ts Show resolved Hide resolved
@jarebudev
Copy link
Contributor Author

Sorry for the delay again, I had quite busy days. This looks really good! :) I added a comment regarding the type checking and conversion, please have a look if you agree. Tomorrow I will have another look but this already looks pretty good.

Edit: I will have a look at the failing tests tomorrow too.

Thanks!
I've added a change to remove the extra cast.

I noticed that the CI is failing and I was unable to replicate the build failures locally. Have I overlooked something?

@jarebudev jarebudev force-pushed the feat/add_unleash_provider branch from 3f741eb to a95aa2b Compare December 22, 2024 22:43
lukas-reining
lukas-reining reviewed Dec 23, 2024
View reviewed changes
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.

The type check looks good!
I added a comment on how you should be able to resolve the problems with the CI. Please have a look, I think once they work and my other comment regarding the versioning is resolved this looks great! :)

.release-please-manifest.json Outdated Show resolved Hide resolved
libs/providers/unleash-web/package.json Outdated Show resolved Hide resolved
@jarebudev
Applied suggested package.json changes to resolve CI building. Correc…
1e27396 
…ted unleash package version mismatches

Signed-off-by: jarebudev <[email protected]>
@jarebudev
applied linting fixes
a0229d9 
Signed-off-by: jarebudev <[email protected]>
lukas-reining
lukas-reining approved these changes Dec 24, 2024
View reviewed changes
Copy link
Member

@lukas-reining lukas-reining left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you @jarebudev!
This looks good to me now!
I will merge this in the next days if no objections will come up until then.

beeme1mr
beeme1mr approved these changes Jan 2, 2025
View reviewed changes
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.

Looks good so far. My only concern is the log level but it's not a blocker for me. Thanks!

libs/providers/unleash-web/README.md Outdated Show resolved Hide resolved
libs/providers/unleash-web/src/lib/unleash-web-provider.ts Outdated Show resolved Hide resolved
This was referenced Jan 7, 2025
Closed
Merged
github-merge-queue bot pushed a commit to open-feature/openfeature.dev that referenced this pull request Jan 7, 2025
@jarebudev
added unleash web JS provider (#902)
1f8375f 
## This PR
adds the unleash web provider for JS

- adds this new feature

### Related Issues

Closes #901 

### Notes

pending merge of this PR into js-sdk-contrib -
open-feature/js-sdk-contrib#1105

### Follow-up Tasks
n/a

### How to test
n/a

Signed-off-by: jarebudev <[email protected]>
@toddbaert
fixup: submodules
6c108e0 
Signed-off-by: Todd Baert <[email protected]>
toddbaert
toddbaert approved these changes Jan 10, 2025
View reviewed changes
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.

Approved - I just reverted some submodule changes in flagd, which I'm sure weren't intentional.

@toddbaert toddbaert merged commit 19accf8 into open-feature:main Jan 10, 2025
6 checks passed
@github-actions github-actions bot mentioned this pull request Jan 10, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukas-reining lukas-reining lukas-reining approved these changes

@toddbaert toddbaert toddbaert approved these changes

@beeme1mr beeme1mr beeme1mr approved these changes

Assignees

@beeme1mr beeme1mr

@jarebudev jarebudev

@toddbaert toddbaert

@lukas-reining lukas-reining

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add unleash provider
4 participants
@jarebudev @lukas-reining @beeme1mr @toddbaert