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!: implement config cat web provider #918

Merged
merged 28 commits into from
Jul 21, 2024
Merged

feat!: implement config cat web provider #918

merged 28 commits into from
Jul 21, 2024

Conversation

lukas-reining
Copy link
Member

@lukas-reining lukas-reining commented May 25, 2024 •
edited
Loading

This PR

Implements ConfigCat Web Provider as discussed here #816.
It mainly moves the context and result mapping logic to a shared package and implements the usage of snapshots for the web-sdk as discussed with @adams85.

@github-actions github-actions bot requested review from beeme1mr and toddbaert May 25, 2024 22:00
@lukas-reining lukas-reining force-pushed the feat/config-cat-web branch 5 times, most recently from 258a9b1 to 30921a7 Compare May 25, 2024 22:32
@lukas-reining lukas-reining marked this pull request as ready for review May 25, 2024 22:36
@lukas-reining lukas-reining requested a review from a team as a code owner May 25, 2024 22:36
@lukas-reining
Copy link
Member Author

@adams85 it took some time to find time to implement, but here it is, as we discussed in #816.
I could not add you as a reviewer, because you are not part of the org. If you like to join the org, I can add you :)
Would be happy about a review.

adams85
adams85 reviewed May 27, 2024
View reviewed changes
Copy link
Contributor

@adams85 adams85 left a comment

Choose a reason for hiding this comment

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

Hey @lukas-reining,

Awesome news! I did a quick review.

libs/providers/config-cat/package.json Outdated Show resolved Hide resolved
libs/providers/config-cat-web/package.json Outdated Show resolved Hide resolved
libs/shared/config-cat-core/src/lib/context-transformer.ts Show resolved Hide resolved
libs/shared/config-cat-core/src/lib/result-mapping.ts Outdated Show resolved Hide resolved
libs/shared/config-cat-core/src/lib/result-mapping.ts Outdated Show resolved Hide resolved
libs/providers/config-cat-web/src/lib/config-cat-web-provider.ts Outdated Show resolved Hide resolved
@toddbaert
Copy link
Member

This is my only concern.

lukas-reining and others added 9 commits July 11, 2024 21:56
@lukas-reining
feat: implement config cat web provider
e103bd6 
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining @adams85
Update libs/providers/config-cat/package.json
cfb1b61 
Co-authored-by: adams85 <[email protected]>
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining @adams85
Update libs/providers/config-cat-web/package.json
45b9fb5 
Co-authored-by: adams85 <[email protected]>
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining
feat: use config cat node for server
41f2a60 
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining @adams85
Update libs/shared/config-cat-core/src/lib/result-mapping.ts
7554e11 
Co-authored-by: adams85 <[email protected]>
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining @adams85
Update libs/shared/config-cat-core/src/lib/result-mapping.ts
e3d994f 
Co-authored-by: adams85 <[email protected]>
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining @beeme1mr
Update README.md
ec1dfa9 
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining
feat: only allow AutoPoll for ConfigCat Web and change to new context…
9c712ae 
… mapping

Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining
chore: update package.lock
004062c 
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining
Copy link
Member Author

lukas-reining commented Jul 11, 2024 •
edited
Loading

Hey @adams85, took me quite some time to get to it but I just changed the two things you proposed :)
Would be happy about a re-review!
I changed the mapping to support your new typings for the User object and only allow AutoPoll for the web sdk.
Marking this as breaking is the User mapping changed.

@lukas-reining lukas-reining changed the title feat: implement config cat web provider feat!: implement config cat web provider Jul 11, 2024
@lukas-reining
feat: fix formatting
66960da 
Signed-off-by: Lukas Reining <[email protected]>
lukas-reining and others added 3 commits July 15, 2024 18:18
@lukas-reining @adams85
Update config-cat-provider.ts
210f17e 
Co-authored-by: adams85 <[email protected]>
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining @adams85
Update config-cat-web-provider.ts
87c3476 
Co-authored-by: adams85 <[email protected]>
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining @adams85
Update config-cat-web-provider.ts
a98ad0c 
Co-authored-by: adams85 <[email protected]>
Signed-off-by: Lukas Reining <[email protected]>
toddbaert
toddbaert approved these changes Jul 17, 2024
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! Nice work. Please just carefully consider this: https://github.com/open-feature/js-sdk-contrib/pull/918/files#r1681377307 (I could be missing something).

adams85
adams85 reviewed Jul 17, 2024
View reviewed changes
Copy link
Contributor

@adams85 adams85 left a comment

Choose a reason for hiding this comment

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

Spotted some outdated info in README.md files.

libs/providers/config-cat-web/README.md Outdated Show resolved Hide resolved
libs/providers/config-cat-web/README.md Outdated Show resolved Hide resolved
libs/providers/config-cat-web/README.md Outdated Show resolved Hide resolved
libs/providers/config-cat/README.md Outdated Show resolved Hide resolved
lukas-reining and others added 4 commits July 17, 2024 19:15
@lukas-reining @adams85
Update README.md
f6e4e98 
Co-authored-by: adams85 <[email protected]>
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining @adams85
Update README.md
a6ad386 
Co-authored-by: adams85 <[email protected]>
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining @adams85
Update README.md
bf91148 
Co-authored-by: adams85 <[email protected]>
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining
fix: fix README and use updateBuildableProjectDepsInPackageJson
1fb34fc 
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining
fix: fix imports
8ff2466 
Signed-off-by: Lukas Reining <[email protected]>
@lukas-reining lukas-reining merged commit e280014 into main Jul 21, 2024
6 checks passed
This was referenced Jul 21, 2024
Merged
Merged
Merged
@toddbaert toddbaert deleted the feat/config-cat-web branch July 23, 2024 14:06
@lukas-reining lukas-reining mentioned this pull request Oct 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adams85 adams85 adams85 approved these changes

@toddbaert toddbaert toddbaert approved these changes

@laliconfigcat laliconfigcat Awaiting requested review from laliconfigcat

@beeme1mr beeme1mr Awaiting requested review from beeme1mr

Assignees

@beeme1mr beeme1mr

@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.
4 participants
@lukas-reining @toddbaert @beeme1mr @adams85