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

Need less entries in require.rb #818

Merged
merged 5 commits into from Mar 3, 2022
Merged

Need less entries in require.rb #818

merged 5 commits into from Mar 3, 2022

Conversation

paracycle
Copy link
Member

Motivation

Contributes to #429

As things stand right now, we need to add all gems that are marked as require: false in the Gemfile to the tapioca/require.rb file manually. If we made Bundler load those gems by default, we would not need any of those entries.

Implementation

We are able to get Bundler to load gems by tricking it to think that gems marked as require: false are actually require: true. We do that by intercepting the autorequire attr reader and changing all [] (which mean require: false) values to nil (which mean require: true).

Tests

Added a test with a gem explicitly marked as require: false, and verified that the gem is loaded and Tapioca can generate an RBI for it without needing a tapioca/require.rb entry.

@paracycle paracycle requested a review from a team February 16, 2022 22:19
@paracycle paracycle added the enhancement New feature or request label Feb 16, 2022
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work with gems that aren't supposed to be loaded by default? For example fakefs?

An alternative solution to having those gems as require: false could be to pass them into the exclude config. I took a brief look and feels like the exclude might be handled after loading all the dependencies with bundler? If that's the case I don't see how we can circumvent fakefs issue with this feature.

@paracycle
Copy link
Member Author

paracycle commented Feb 22, 2022

Will this work with gems that aren't supposed to be loaded by default? For example fakefs?

Indeed, gems like fakefs would definitely be a problem. Thanks for raising this.

An alternative solution to having those gems as require: false could be to pass them into the exclude config. I took a brief look and feels like the exclude might be handled after loading all the dependencies with bundler? If that's the case I don't see how we can circumvent fakefs issue with this feature.

Agreed, we should probably not change the default auto-require behaviour of any gem that is excluded from RBI generation.

paracycle and others added 5 commits March 2, 2022 22:27
Co-authored-by: Alexandre Terrasa <583144+Morriar@users.noreply.github.com>
Co-authored-by: Alexandre Terrasa <583144+Morriar@users.noreply.github.com>
Co-authored-by: Alexandre Terrasa <583144+Morriar@users.noreply.github.com>
Co-authored-by: Alexandre Terrasa <583144+Morriar@users.noreply.github.com>
Co-authored-by: Alexandre Terrasa <583144+Morriar@users.noreply.github.com>
@paracycle
Copy link
Member Author

@KaanOzkan We implemented the exclusion of excluded gems from being forcibly required and updated the test to check for that too. Do you mind re-reviewing?

@paracycle paracycle merged commit 83a55f6 into main Mar 3, 2022
@paracycle paracycle deleted the uk-at-less-require branch March 3, 2022 22:55
@shopify-shipit shopify-shipit bot temporarily deployed to production May 13, 2022 23:08 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.

None yet

3 participants