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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

feat: add unleash web provider #1105

wants to merge 14 commits into from

Conversation

jarebudev
Copy link

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
initial unleash provider - wip
21c7f11 
Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
@jarebudev
merge in main and resolve conflict
1ad0bea 
Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
@jarebudev
added evaluation tests
ca52bf1 
Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
@jarebudev
added event tests
2678eec 
Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
@jarebudev
added documentation, extended rather than duplicate unleash config op…
7faed09 
…tions

Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
@jarebudev
Merge branch 'main' into feat/add_unleash_provider
711a41a 
Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
@jarebudev
linting fixes applied
7c8a41b 
Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
@jarebudev jarebudev requested a review from a team as a code owner December 5, 2024 23:32
@jarebudev
added context change tests, tsconfig options
10f70d1 
Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
@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
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
}

if (evaluatedVariant.name === 'disabled') {
value = defaultValue as T;
Copy link
Member

Choose a reason for hiding this comment

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

Is the cast necessary? I think it should already be of type T.

Copy link
Author

Choose a reason for hiding this comment

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

almost certainly not but I will double check!

libs/providers/unleash-web/src/lib/unleash-web-provider.ts Show resolved Hide resolved
@jarebudev
Copy link
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
removed unnecessary cast. Added component owner
2cee133 
Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
@jarebudev
added checks to ensure that returned variant matches requested flag t…
a95aa2b 
…ype and additional tests

Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
@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 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 <23311805+jarebudev@users.noreply.github.com>
@jarebudev
applied linting fixes
a0229d9 
Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
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.

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

@beeme1mr beeme1mr Awaiting requested review from beeme1mr

@toddbaert toddbaert Awaiting requested review from toddbaert

Assignees

@beeme1mr beeme1mr

@jarebudev jarebudev

@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
3 participants
@jarebudev @lukas-reining @beeme1mr