Filter engines in application directory so they are not loaded

[wildmaples]

Motivation

Solves: #993

Also fixes an internal Shopify/shopify issue

Implementation

As Rafael suggested in #993 (comment), add a filter to reject engines that are part of the application directory

Tests

Change is part of the application loading - can this be tested?

[rafaelfranca]

Looks good to me but I can't answer for the "can this be tested" part.

[Morriar]

can it be tested?

It may. In https://github.com/Shopify/tapioca/blob/main/spec/tapioca/cli/gem_spec.rb we use SpecWithProject to create a mock project that we pass to Tapioca.

I wonder if we could create a small repro with an engine and make sure we do not load it?

[wildmaples]

So I duplicated #gem_in_app_dir? from Gemspec in 2f0d6a6 because I wanted to confirm the approach works.

This is, however, duplicated code.Loader is does not have convenient access to Gemspec unfortunately, and I don't think it's a good idea for it to have access to the instances of Gemspec. Both classes have different jobs and are part of different stages in Tapioca's execution. With that, another option is to extract the logic into a utility module that can be used by both Loader and Gemspec. What do you think?

[rafaelfranca]

With that, another option is to extract the logic into a utility module that can be used by both Loader and Gemspec

I think that is a valid way to share code. If you can use composition for this it would be better in my opinion but also totally fine to use inheritance.

[Morriar]

I think we can extract those methods under a new helpers/gem_helper module and include it in both places.

@rafaelfranca for composition are you suggesting making it a class that is instantiated in both places? It doesn't seem like those methods require any state?

[rafaelfranca]

Doesn't need to be a class, it could be a module we pass around, but not a big deal

[wildmaples]

Alright folks, I've addressed all the comments and added a spec for the change. I've also tophat the latest commit against Shopify/shopify -- tapioca gem --all runs successfully. I would love to get this in today so tomorrow's RBI generation run will be successful.

[Morriar]

tapioca gem --all runs successfully.

It's also good to check if srb tc finished without any unexpected errors. This may be the sign we're missing some constants that should be generated.

[wildmaples]

Thanks for the review! I ran srb tc and there are errors so I'm gonna investigate to see if they are real changes before merging this.

Edit: @KaanOzkan is taking over to do this

[DavidPD]

Is there any possibility that this fix could be backported to the 0.7.x version as well? Unfortunately my team is stuck in Ruby 2.6 for the time being and 0.7.x is the latest version of tapioca we can use. It looks like the fix was ported back as far as 0.8.x already.

I may also try to fork and fix this myself for 0.7.x so if I get that working I could submit a PR.

[KaanOzkan]

@DavidPD Since we don't support Ruby 2.6 (EOL) I think this would set a bad precedent for the repo. However, I'm confident that we won't backport anything else to 0.7.x so if you were to point to your personal fork you wouldn't be at a disadvantage compared to sticking to tapioca 0.7.x.

So I suggest you fork the project, backport the fix and point tapioca to your fork. This might even be the better long term solution if you're sticking to Ruby 2.6 for a while since you won't be dependent on us for other backports. In the future when you can upgrade Ruby you can point back to RubyGems releases.

I'm very sorry about the inconvenience, hopefully you can utilize latest Tapioca in no time since there have been a ton of improvements 🙂

[DavidPD]

@KaanOzkan Thanks for the update. Yeah that's pretty much what we're doing short-term, and hopefully not too far down the road we can get our project somewhat up-to-date.