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

feat: typesafety for client options (new surface only) #450

Merged
merged 29 commits into from
Sep 14, 2023

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Mar 31, 2023

Add classes to wrap our current typeless array for Client Options. There is a different "Options" class for each "layer" of options (e.g. nested array), so that we have typehinted classes with corresponding docs instead of untyped associative arrays. This will allow us to have consistent options across our various classes, including handwritten clients.

An overview of the design decisions

  1. I implemented ArrayAccess in OptionsTrait so that the getters do not show up in the docs or IDEs. This is just a nice convenience factor, since the getters shouldn't be used or be considered very important. It also causes less churn for our internal code, since we were previously using array access.
  2. I made all the setters public so they do show up in IDEs and docs. These could be made private, as the most common API will be to just call the constructor with an array. But I thought it seemed nicer for the documentation and API.
  3. I didn't add typehints for the Options classes public methods (e.g., customers could only use these classes in their code if they called toArray() before passing them anywhere). This is because I didn't want to widen (e.g. remove) the array typehints (as union types aren't available until PHP 8.0! 😡). If we decide to do it this way (we can always widen later), we should at least add a reference (e.g. @see ClientOptions), etc, to everywhere it's applicable, so users can find the docs.
  4. I left all strings nullable. We could default to empty strings, but I feel like null says something different. I did not allow arrays/booleans to be nullable, because I don't think that distinction means anything (and it just makes the code look sloppier).

cc @saranshdhingra and @noahdietz - This is a draft! I am not hard-set on any of these design decisions! I would still love your feedback!

  • Only use in V2 client surfaces
  • Add CallOptions
  • Add tests
  • Add better docs explaining its usage
  • Get design approval
  • drop support for PHP 7.4

@bshaffer bshaffer marked this pull request as ready for review June 30, 2023 18:56
@bshaffer bshaffer requested review from a team as code owners June 30, 2023 18:56
@bshaffer bshaffer requested a review from noahdietz July 6, 2023 17:39
@bshaffer bshaffer added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 6, 2023
@bshaffer
Copy link
Contributor Author

bshaffer commented Jul 6, 2023

Added Do not merge as I'd like to wait one week after we release #470 in order to mitigate churn

@bshaffer bshaffer removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 15, 2023
@bshaffer bshaffer changed the title feat: add classes for client options feat: typesafety for client options (new surface only) Aug 23, 2023
@bshaffer bshaffer merged commit 21550c5 into main Sep 14, 2023
@bshaffer bshaffer deleted the add-options-classes branch September 14, 2023 18:14
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.

3 participants