-
Notifications
You must be signed in to change notification settings - Fork 83
[FSSDK-11510] add validation to factories #1060
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
Conversation
also updated createInstance to throw in case of validation errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds input validation to various factories, refactors event processor exports, and changes createInstance
to throw on invalid configs (removing null returns).
- Introduce
validateEventDispatcher
andvalidateErrorHandler
guards in factories - Refactor forwarding processor export and update factory functions/tests
- Change
createInstance
signature to always return aClient
and throw on validation errors
Reviewed Changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
lib/index.browser.tests.js | Commented out outdated config validation test; update coverage needed |
lib/event_processor/event_processor_factory.ts | Added validateEventDispatcher , updated extractors, added factory export |
lib/event_processor/forwarding_event_processor.ts | Exported class instead of internal factory, removed standalone factory |
lib/event_dispatcher/event_dispatcher_factory.ts | Added validateRequestHandler before dispatcher instantiation |
lib/error/error_notifier_factory.ts | Added validateErrorHandler and updated extractor to return Maybe |
lib/entrypoint.universal.test-d.ts & lib/entrypoint.test-d.ts | Updated createInstance return type from `Client |
lib/client_factory.ts | Removed configValidator.validate , made getOptimizelyInstance throw |
Comments suppressed due to low confidence (6)
lib/index.browser.tests.js:137
- This test has been commented out, reducing coverage for config validation. Since createInstance now throws on validation errors, add tests to assert the expected throw behavior.
// it('should not throw if the provided config is not valid', function() {
lib/event_dispatcher/event_dispatcher_factory.ts:21
- Added
validateRequestHandler
guard but no tests cover invalid handlers; consider adding tests to verify errors are thrown for bad inputs.
import { validateRequestHandler } from '../../utils/http_request_handler/request_handler_validator';
lib/event_processor/event_processor_factory.react_native.spec.ts:74
- The
import { get } from 'http'
is unused in this test; remove it to avoid unnecessary dependencies.
import { get } from 'http';
lib/event_processor/event_processor_factory.browser.spec.ts:53
- This HTTP import isn’t used in browser tests; please remove
import { get } from 'http'
.
import { get } from 'http';
lib/event_processor/event_processor_factory.node.spec.ts:48
- Unnecessary import of
get
from 'http'; remove it to keep tests focused and clean.
import { get } from 'http';
lib/client_factory.ts:34
- The removal of
configValidator.validate(config)
may allow invalid configs to be accepted, causing runtime issues. Consider reintroducing explicit validation or handling invalid inputs early.
const getOptimizelyInstance = (config: OptimizelyFactoryConfig): Client => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Changes in the right direction!
also updated createInstance to throw in case of validation errors
Test plan
Issues