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

RFC: Static sorbet/RBI annotations for resources and methods #1479

Open
helenye-stripe opened this issue Oct 30, 2024 · 2 comments
Open

RFC: Static sorbet/RBI annotations for resources and methods #1479

helenye-stripe opened this issue Oct 30, 2024 · 2 comments

Comments

@helenye-stripe
Copy link
Contributor

helenye-stripe commented Oct 30, 2024

👋 Hello all Ruby devs! We're seeking feedback on a feature we're working on. The SDKs team will be adding optional static type annotation RBIs to our stripe-ruby library. We do not plan to introduce runtime dependencies or breaking changes for existing users on the resource-based pattern, and we want to continue to support users who are using extra parameters (beta users). We are seeking feedback on our proposed approach.

Resource Fields

We will add explicit attribute definitions in our resources, while still leaving the behavior where a resource will dynamically add any attribute methods that it can when deserialized, so any field returned on the API can be accessed. An example:

# account.rbi snippet
class Account < APIResource
  class SupportAddress < StripeObject
    sig { returns T.nilable(String) }
    attr_reader :city
  
    sig { returns T.nilable(String) }
    attr_reader :country
  
    sig { returns T.nilable(String) }
    attr_reader :line1
  
    sig { returns T.nilable(String) }
    attr_reader :line2
  end

  sig { returns Integer }
  attr_reader :created

  sig { returns T.nilable(SupportAddress) }
  attr_reader :support_address
end

# account.rb
class Account < APIResource
  class SupportAddress < StripeObject
    attr_reader :city
    attr_reader :country
    attr_reader :line1
    attr_reader :line2
  end

  attr_reader :created
  attr_reader :support_address
end

# usage
acc = Stripe::Account.retrieve("acct_123")
acc.created # will pass with Sorbet checks
acc.beta_field # will work at runtime but Sorbet checks will need to be suppressed if enabled 

Method Parameters

We will add a RequestParameter object with subclasses for each method, both for resource and service methods. The object will have attributes defined, and we will accept the parameter for all methods in the SDK, while still accepting untyped hashes. An example, using Stripe::Account.reject:

# account.rbi
class Account
  class RejectParams
    sig {returns String}
    attr_accessor :reason
    sig {returns T.nilable(T.Array[String])}
    attr_accessor :expand

    def initialize(...)
      ... # init
    end
  end

  sig { params(params: T.any(RejectParams, T::Hash[T.untyped, T.untyped]), opts: T.any(RequestOptions, T::Hash[T.untyped, T.untyped])).returns(Stripe::Account)
  def self.create(params = {}, opts = {}); end
end

# secret.rb
class Account
  class RejectParams
    attr_accessor :name
    attr_accessor :expires_at
    attr_accessor :scope
  end
    class Scope
      attr_accessor :type
      attr_accessor :user
    end
  end

  def self.create(params = {}, opts = {}); end
end

Conclusion

Existing users shouldn't be affected by our addition of types. Users who currently use sorbet for their own projects but not for Stripe can partially migrate, as all methods will support both parameter objects and hashes.

When shipping types, we will not include any new runtime dependencies (ie inline annotations). We will maintain support for Ruby 2.3+.

@helenye-stripe helenye-stripe self-assigned this Oct 30, 2024
@helenye-stripe helenye-stripe changed the title Static sorbet/RBI annotations for resources and methods Static sorbet/RBI annotations for resources and methods - seeking feedback Oct 30, 2024
@xavdid-stripe xavdid-stripe pinned this issue Oct 31, 2024
@xavdid-stripe xavdid-stripe changed the title Static sorbet/RBI annotations for resources and methods - seeking feedback RFC: Static sorbet/RBI annotations for resources and methods Oct 31, 2024
@olivier-thatch
Copy link

Hello! Former maintainer of the library here (I was @ob-stripe) and current user of both Sorbet and the library at my new gig.

First off, yay! Very excited to finally have first-party types 🎉 🥳 🎈

(You're probably already aware but there are unofficial Sorbet types in Tapioca's repo: https://github.com/Shopify/rbi-central/blob/main/rbi/annotations/stripe.rbi)

The signatures for resource fields look fine, but I don't love the approach for method parameters. Unless I misunderstand, using types for request params would require a code change to use the new Params classes, e.g.:

# without types
Stripe::Account.reject(reason: "foo")

# with types
Stripe::Account.reject(Stripe::Account::RejectParams.new(reason: "foo"))

This makes the code more cumbersome to read, and has a (slight) performance impact since more allocations are needed, etc.

Instead, I wonder if using shapes would provide a better experience:

# account.rbi
class Account
  RejectParamsType = T.type_alias { { reason: String } }

  sig { params(params: RejectParamsType).returns(Stripe::Account) }
  def self.create(params = {}); end
end

# client code
Stripe::Account.reject(reason: "foo")

But there are some problems of this approach:

  1. you can't do T.any(RejectParams, T::Hash[T.untyped, T.untyped]) as Sorbet would never report any errors: if the hash doesn't match the RejectParams shape, it would always match T::Hash[T.untyped, T.untyped]. I think you'd have to define different method names for the typed and untyped version... Not great :(
  2. shapes have been considered an "experimental feature" in Sorbet forever, so you may not want to use them at all

@helenye-stripe
Copy link
Contributor Author

helenye-stripe commented Nov 12, 2024

👋 @olivier-thatch Thanks for stopping by at stripe again and giving us feedback! 😄

I consulted with the Sorbet maintainers about shapes and I do not think they would be the best option here, unfortunately. There are several reasons:

  • Shapes are not well-supported, and haven't been updated since they were introduced years ago.
  • We are aiming to not introduce any major runtime breaking changes for users who do not opt into types. Type aliases unfortunately have to be defined at runtime for users with runtime dependencies, which would not allow the flexible support we're looking for.
  • Statically, Sorbet allows additional keys in shapes. However, at runtime, shapes cannot have additional keys. Since we support betas with extra parameters, and gated parameters, this would not

I understand the migration path isn't ideal for parameters with requiring a new object, but it would be hard to both define a type for parameters and have its rollout be nonbreaking, but I'd be glad to here any additional feedback you may have here!

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

2 participants