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

Add support for --include-dependencies flag #1548

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

LTe
Copy link
Contributor

@LTe LTe commented Jun 29, 2023

Motivation

In my project we have huge Gemfile with a lot of gems. We tried to type some of the files, but we do not need all of the gems to be generated. We tried to use tapioca gems [name_of_the_gem] but it was generating only this gem (without dependencies). We tried exclude but list was huge.

I think we should have --include-dependencies flag that generate RBIs for dependencies as well

Implementation

I added new flag for CLI that will pick gems and dependencies from the list.

Tests

I added integration tests

@LTe LTe requested a review from a team as a code owner June 29, 2023 14:53
@LTe LTe requested review from KaanOzkan and vinistock June 29, 2023 14:53
@KaanOzkan
Copy link
Contributor

Hey @LTe, could you elaborate on the reasoning behind only generating a handful of dependencies for the lifetime of the project? Is it performance related? We also have a large Gemfile but didn't reach for this feature so I'm trying to understand.

@LTe
Copy link
Contributor Author

LTe commented Jun 30, 2023

I think there is a couple reasons why I want this kind of feature in tapioca.

Currently my Gemfile.lock include about ~630 gems

Without only With only
Time 447,81s 30,18s
Size (du sorbet/rbi/gems) 22M 1,4M
Count (ls -l sorbet/rbi/gems | wc -l) ~560 ~20
wc -l rbi/todo.rbi 80 4

As you can see there is significant difference in performance.

Do I really need 560 generated RBIs? Most of the missing constants inside todo.rbi are from internal gems and this does not influence typechecking in my application (at least for now).

I want this feature just for flexibility, to have control what I will generate for my application. I can of course do similar thing with external script, but I think this feature should be included in tapioca.

To be honest 560 files where 541 is not really needed can be hard to sell other team members. I think that reducing the number of files (at least on the beginning of adoption) generated by 'tapioca' may increase the number of projects that will use tapioca and sorbet.

So in summary, I want this feature in tapioca because of:

  • performance
  • flexibility
  • in order to more easily convince the unconvinced to adopt a sorbet

@KaanOzkan
Copy link
Contributor

This is an interesting approach but I'm still not convinced that this is a good way of using Tapioca in the long term: Generating RBIs only for gem constants that are referenced in the application. What you want with this PR can be achieved with supplying arguments to tapioca gem, only difference is this PR will figure out the direct dependencies for you. I think we have to discuss as a team if that's an acceptable use case to recommend through this flag.

Biggest issue I have with this setup is the need for manual tapioca config changes every time the application starts using a new gem or starts referencing new part of a gem, it's an extra manual step to be done by the user. Knowing which gem to load can also get complex if the gem's RBIs changes conditionally based on the existence of another gem. Users will need to inspect gem's code.

Side note: why does this feature need to generate RBIs for sub-dependencies that aren't referenced by the application if you're using a todo.rbi? Is it because some gems have different RBIs if their sub-dependencies are loaded?

We have a similar number of gems and in practice we rarely run bin/tapioca gem --all, as a result performance isn't a big concern. We update individual RBIs upon version bumps and run bin/tapioca gem --all from a scheduled build once a week. Maybe a change in workflow would help performance concerns. We don't run tapioca gem --verify on CI, just dsl --verify and srb tc.

@KaanOzkan
Copy link
Contributor

We discussed this internally and while --only flag doesn't seem too beneficial for rest of the community we could add a --include-dependencies option for the default tapioca gem usage, ie. bin/tapioca gem foo bar baz --include-dependencies. This will support your use case and can be useful to avoid regenerating every outdated RBI using tapioca gem in certain situations such as dependabot PRs.

What do you think about switching the implementation to that? Biggest difference would be to iterate recursively over all dependencies and keeping track of them in a set instead of limiting to one level. We'll also have to ensure this can't be used alongside --all.

@paracycle I'm curious if you have opinions on this approach.

@LTe
Copy link
Contributor Author

LTe commented Aug 17, 2023

@KaanOzkan I updated pull request and implemented --include-dependencies flag.

@LTe LTe changed the title Add support for --only flag Add support for --include-dependencies flag Aug 17, 2023
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/tapioca/cli.rb Show resolved Hide resolved
lib/tapioca/commands/abstract_gem.rb Outdated Show resolved Hide resolved
lib/tapioca/commands/abstract_gem.rb Outdated Show resolved Hide resolved
lib/tapioca/commands/abstract_gem.rb Outdated Show resolved Hide resolved
@KaanOzkan
Copy link
Contributor

@LTe We don't have squash and merge enabled in tapioca so the commit history will also need some cleaning up. Thank you!

@LTe LTe force-pushed the add-support-for-only branch 2 times, most recently from bef007b to 2fa0bc7 Compare August 22, 2023 08:25
@LTe LTe requested a review from KaanOzkan August 22, 2023 08:56
@LTe
Copy link
Contributor Author

LTe commented Aug 22, 2023

@KaanOzkan I updated the code and documentation.

Do you think that this PR can be related to #1576?

README.md Outdated Show resolved Hide resolved
@KaanOzkan
Copy link
Contributor

Do you think that this PR can be related to #1576?

Good shout! I think --include-dependencies is a good workaround for most cases of what the issue describes. I don't know if it can solve all the occurrences in practice because RBI dependency is not same as gem dependencies. Gem X can load different things based on existence of gem Y without depending on Y. But this flag would solve the example issue I described. Because of that maybe it's worth enabling this by default. I'm curious what the team thinks although we don't run into #1576 often and it could be resolved by a more sophisticated tracking solution. For this PR I think we can keep the default as false and re-consider after using this flag, it will also unblock you faster.

lib/tapioca/commands/abstract_gem.rb Outdated Show resolved Hide resolved
lib/tapioca/commands/abstract_gem.rb Outdated Show resolved Hide resolved
@LTe LTe requested a review from vinistock August 24, 2023 08:18
lib/tapioca/commands/abstract_gem.rb Outdated Show resolved Hide resolved
This commit introduces a feature that allows users to generate (RBI)
files for dependencies of specified gems. A new `--include-dependencies`
option has been added to the `gem` command in the Tapioca CLI.

When this option is used with specified gem names, RBI files will also be
generated for their dependencies. Exception handling has been improved to
display an error message if the `--include-dependencies` option is used without
specifying any gems.

Additionally, the default value of `include_dependencies` has been set to `false`
to ensure compatibility with existing use cases. The README, CLI help
documentation, and config file have been updated accordingly to reflect these
changes.

This feature provides more flexibility to users who want to generate RBIs for a
gem along with its dependencies, enhancing the overall usability of Tapioca.

Co-authored-by: Vinicius Stock <vinistock@users.noreply.github.com>
@KaanOzkan KaanOzkan added the enhancement New feature or request label Aug 24, 2023
@KaanOzkan KaanOzkan merged commit 104311f into Shopify:main Aug 24, 2023
15 checks passed
@shopify-shipit shopify-shipit bot temporarily deployed to production September 13, 2023 22:55 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants