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

Set default if factory prop is set #833

merged 1 commit into from Mar 3, 2022


Copy link

@jeffcarbs jeffcarbs commented Feb 28, 2022


Explained more in Shopify/rbi#122, but basically T::Struct fields have a factory option, which is basically the same as default but takes a proc so the value can be set dynamically. Like default, if a prop/const has a factory set you don't have to pass a value in the initializer.

Tapioca does not handle the factory option so based on the generated RBI a field with a factory would seem to be required in the initializer when it actually isn't. This has just come up for us with our first usage of this option in one of our internal gems so we've had to monkey-patch tapioca for now.


I noticed that when we see immutable: true we put a Const, even if the field may have been defined via prop :x, Integer, immutable: true. It gives the same effect to the type checker while not necessarily being identical to the source code.

I took the same approach here for factory. For type-checking purposes, we just want sorbet to know if a field has a factory it will be optional in the initializer. So where we were previously checking if a default was defined and setting default: T.unsafe(nil) in the RBI, we now also check for factory and do the same (set default: T.unsafe(nil) in the RBI).


Added a test that was red before and green after.

Copy link

@paracycle paracycle left a comment

Thanks, this looks reasonable.

@paracycle paracycle merged commit 9ef7521 into Shopify:main Mar 3, 2022
5 checks passed
@jeffcarbs jeffcarbs deleted the tstruct-factory branch Mar 3, 2022
@shopify-shipit shopify-shipit bot temporarily deployed to production May 13, 2022 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants