Skip to content

[FSSDK-11510] refactor unsupported factories to not throw #1056

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

Merged
merged 1 commit into from
May 16, 2025

Conversation

raju-opti
Copy link
Contributor

Summary

  • previously we were throwing from unsupported factories. This PR changes them to return undefined instead. It will make writing isomorphic apps easier.

Test plan

Issues

  • FSSDK-11510

Copy link

@Copilot Copilot AI left a 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 refactors unsupported factories so they now return undefined instead of throwing errors, making it easier to write isomorphic apps.

  • Updated VuidManager extraction and wrapping functions to use the Maybe type.
  • Modified platform-specific factories (Node, React Native, Browser) to return undefined for unsupported operations.
  • Adjusted tests and type definitions to verify the new behavior.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
lib/vuid/vuid_manager_factory.ts Updated extract and wrap functions to use Maybe for unsupported cases.
lib/vuid/vuid_manager_factory.node.ts Changed createVuidManager to return an undefined-wrapped value instead of throwing an error.
lib/vuid/vuid_manager_factory.node.spec.ts Updated tests to assert that an undefined value is returned.
lib/vuid/vuid_manager_factory.browser.spec.ts Updated describe block naming to reflect the change in behavior.
lib/index.react_native.ts Modified getSendBeaconEventDispatcher to return undefined instead of throwing an error.
lib/index.node.ts Modified getSendBeaconEventDispatcher to return undefined.
lib/index.browser.ts Updated getSendBeaconEventDispatcher signature to include undefined.
lib/entrypoint.test-d.ts Adjusted the type of getSendBeaconEventDispatcher to return a Maybe.

@coveralls
Copy link

Coverage Status

coverage: 80.192%. remained the same
when pulling 9035782 on raju/factory_throw
into 1cd028d on master.

Copy link
Contributor

@junaed-optimizely junaed-optimizely left a comment

Choose a reason for hiding this comment

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

LGTM! Nice improvement!

@raju-opti raju-opti merged commit b70e79e into master May 16, 2025
17 checks passed
@raju-opti raju-opti deleted the raju/factory_throw branch May 16, 2025 13:27
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.

None yet

3 participants