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!: Allow ConfigCat provider to be used in server applications #796

Merged

Conversation

adams85
Copy link
Contributor

@adams85 adams85 commented Mar 14, 2024

This PR

The conficat-js package that the ConfigCat provider is based on is for browser applications only as it uses XMLHttpRequest under the hood. However, the provider might be used in server applications (like it was attempted in a Node.js application here, at around 2:00:00).

For multiplatform scenarios, ConfigCat provides the conficat-js-ssr package (that uses Axios under the hood). This provider should use that instead of conficat-js to provide a more extensive support for various JS platforms.

This PR also fixes another serious shortcoming of the provider: currently it cannot evaluate feature flags without setting targetingKey in the evaluation context. Simple feature flags may not use targeting at all, so requiring a targetingKey to evaluate them is obviously not correct.

Related Issues

n/a

Notes

n/a

Follow-up Tasks

n/a

How to test

n/a

@adams85 adams85 requested a review from a team as a code owner March 14, 2024 12:08
@adams85 adams85 changed the title Allow ConfigCat provider to be used in server applications fix: Allow ConfigCat provider to be used in server applications Mar 14, 2024
@adams85 adams85 force-pushed the fix/configcat-node-server-support branch from e1c6b81 to 1070842 Compare March 14, 2024 12:12
…gCat provider to support non-browser platforms

Signed-off-by: Adam Simon <adam@configcat.com>
@adams85 adams85 force-pushed the fix/configcat-node-server-support branch from 1070842 to ee37eda Compare March 14, 2024 12:15
Signed-off-by: Adam Simon <adam@configcat.com>
@adams85 adams85 force-pushed the fix/configcat-node-server-support branch from ee37eda to ce046ff Compare March 14, 2024 12:16
@lukas-reining lukas-reining changed the title fix: Allow ConfigCat provider to be used in server applications fix!: Allow ConfigCat provider to be used in server applications Mar 14, 2024
@lukas-reining
Copy link
Member

Hey @adams85 thank you this change looks good!
Maybe we should also consider developing a client-sdk provider, as this is a server sdk provider. Which makes you change even more important.
We can release this, but it has to be a breaking change as the API changed.

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.

Looks good to me and thanks for the changes!

@lukas-reining lukas-reining merged commit 190946f into open-feature:main Mar 14, 2024
8 checks passed
@adams85
Copy link
Contributor Author

adams85 commented Mar 20, 2024

Thanks for the release, @lukas-reining. I did a quick test and found that the new version (v0.6.0) works fine.

Maybe we should also consider developing a client-sdk provider, as this is a server sdk provider. Which makes you change even more important.

Since our SSR SDK can handle both targets, I guess the client provider would be more or less just a duplicate of the server one (at least, in our case). I'd avoid maintaining another package if possible.

BTW, NPM packages kind of support multitargeting via the browser field. AFAIK, most of the bundlers support it nowadays. I have plans to look into the viability of this approach because it would be a godsend to us if we could merge our various JS SDK packages (or at least some) into one.

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.

3 participants