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

[FSSDK-9987] fix: Conditional ODP instantiation #895

Closed

Conversation

mikechu-optimizely
Copy link
Contributor

Summary

  • When explicit odpOptions.disabled = true is passed, avoid ODP instantiation
  • Fix Jest test setting in VSCode
  • Lint updates

Test plan

  • Existing unit and e2e test suite are expected to pass

Issues

  • FSSDK-9987

@mikechu-optimizely mikechu-optimizely marked this pull request as ready for review February 6, 2024 20:53
@mikechu-optimizely mikechu-optimizely requested a review from a team as a code owner February 6, 2024 20:53
@coveralls
Copy link

Coverage Status

coverage: 90.39% (-0.02%) from 90.411%
when pulling 5775e15 on mike/FSSDK-9987/performance-refactors
into 961f6a1 on master.

@mikechu-optimizely
Copy link
Contributor Author

Developer review completed.

Copy link
Contributor

@jaeopt jaeopt 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. We need to double check of its side-effects.

For example -

this.logger.error(ERROR_MESSAGES.ODP_FETCH_QUALIFIED_SEGMENTS_FAILED_ODP_MANAGER_MISSING);

They do not want to see many "ERROR"-level logging. Better stay silent except for "ODP disabled" at the init phase.

@mikechu-optimizely
Copy link
Contributor Author

Looks good to me. We need to double check of its side-effects.

For example -

this.logger.error(ERROR_MESSAGES.ODP_FETCH_QUALIFIED_SEGMENTS_FAILED_ODP_MANAGER_MISSING);

They do not want to see many "ERROR"-level logging. Better stay silent except for "ODP disabled" at the init phase.

This is an EXCELLENT catch @jaeopt. Thanks. Let me comb through and find these scenarios.

@mikechu-optimizely
Copy link
Contributor Author

Temp close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants