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: Hooks gracefully return with an error message when optimizely prop is null #154

Merged
merged 5 commits into from
Apr 6, 2022

Conversation

opti-jnguyen
Copy link
Contributor

@opti-jnguyen opti-jnguyen commented Mar 9, 2022

Summary

Enables usage of hooks within the Optimizely Provider without passing in the optimizely prop.

Previously, attempting to use hooks within an Optimizely Provider with a missing optimizely prop led to an error being thrown resulting in a crash.

With this change, hooks now return pessimistic results when the optimizely prop is null.

Addresses this issue:

Also related:

Note:

  • Instead of returning an "Error" message like requested, we will defer to SDK users to handle the pessimistic values.
  • In certain edge cases, this may be a potentially breaking change for users that expect hooks to throw an error when the optimizely prop is null.

Test Plan

Refactored tests that expect hooks to throw errors when Optimizely provider is not present.

@opti-jnguyen opti-jnguyen marked this pull request as ready for review March 16, 2022 16:39
@@ -1,5 +1,5 @@
/**
* Copyright 2018-2019, Optimizely
* Copyright 2022, Optimizely
Copy link
Contributor Author

Choose a reason for hiding this comment

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

2018-2019, 2022

src/hooks.ts Outdated
if (!optimizely) {
throw new Error('optimizely prop must be supplied via a parent <OptimizelyProvider>');
hooksLogger.warn(`Unable to use decision ${flagKey}. optimizely prop must be supplied via a parent <OptimizelyProvider>`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move conditional above overrideAttrs

src/hooks.ts Outdated
hooksLogger.warn(`Unable to use decision ${flagKey}. optimizely prop must be supplied via a parent <OptimizelyProvider>`);
return [
createFailedDecision(flagKey, 'Optimizely SDK not configured properly yet.', {
id: overrides.overrideUserId || null,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Return null for user items bc user depends on Optimizely object

@opti-jnguyen
Copy link
Contributor Author

@zashraf1985 - let me know when you get a chance to review. Thanks in advance!

Copy link
Contributor

@zashraf1985 zashraf1985 left a comment

Choose a reason for hiding this comment

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

Looks Great! Can you change the PR description format to what we usually use like "Summary", "Test Plan" etc.

@opti-jnguyen opti-jnguyen merged commit e92a4c1 into master Apr 6, 2022
@opti-jnguyen opti-jnguyen deleted the jnguyen/hooks-return-error branch April 6, 2022 16:08
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.

2 participants