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

please change your code to throw in error conditions rather than return null #796

Open
olsonpm opened this issue Jan 30, 2023 · 5 comments

Comments

@olsonpm
Copy link

olsonpm commented Jan 30, 2023

How would the enhancement work?

So far I've seen this pattern in createInstance and createUserExperience

  • createInstance should throw instead of return null when an error occurs. i.e. the try/catch should be removed here
  • createUserExperience should throw when input is invalid with an error message explaining why. Returning null gives the devs no clue as to what went wrong or why - just that something went wrong. It is an error and should be treated as such

When would the enhancement be useful?

because returning null in error scenarios doesn't make sense. Devs naturally expect an error to bubble with the proper stack trace and error information rather than the function to return successfully and have to write the error edge case themselves.

@olsonpm olsonpm changed the title please remove try/catch in createInstance please change your code to throw in error conditions rather than return null Jan 30, 2023
@opti-jnguyen
Copy link
Contributor

opti-jnguyen commented Feb 2, 2023

Thanks for raising this issue @olsonpm - nice catch, I think I agree with you on the approach of throwing an exception instead of returning null for these two cases. Second case with createUserExperience might benefit from an additional error log as well.

Will chat with the team internally to verify if there's any contextual significance before proceeding further.

@opti-jnguyen
Copy link
Contributor

@olsonpm - due to this being a breaking change for current users of the SDK, we'll be putting further development of this on the backburner to be done at another date, likely as part of a greater refactor to the JavaScript SDK. Will keep this issue open but unresolved until then.

@olsonpm
Copy link
Author

olsonpm commented Mar 8, 2023

Makes sense. I really appreciate the update

@philBrown
Copy link

@olsonpm - due to this being a breaking change for current users of the SDK, we'll be putting further development of this on the backburner to be done at another date, likely as part of a greater refactor to the JavaScript SDK. Will keep this issue open but unresolved until then.

The change to having createInstance return Client | null instead of just Client in 4.9.2 was already a breaking change that was released as a patch update.

@Tamara-Barum
Copy link

Internal ticket created FSSDK-9572

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

No branches or pull requests

4 participants