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

Load engines properly in both Zeitwerk and Classic modes #1389

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

paracycle
Copy link
Member

@paracycle paracycle commented Feb 9, 2023

Motivation

Until now, our engine loading in Classic mode has been super sketchy, and even our engine loading in Zeitwerk mode was missing some functionality.

This PR improves engine loading so that it:

  1. works both in Classic and Zeitwerk modes,
  2. properly runs engine initializers,
  3. is best effort for a given engine,
  4. isolates engines from failures in loading of other engines.

By running engine initializers, we are able to finally fix #557. However, the fact that we run initializers means that some engines could raise exceptions during load time depending on the existence of certain conditions. For example, Active Storage raises an exception when ActiveStorage::Blob is loaded if it is not configured. This means that applications that have not configured AS will only get abridged RBI file, since we won't be able to load anything past ActiveStorage::Blob.

This is not a problem, though, since if the application has not configured Active Storage, it means they are not really using it anyway, so it should not matter. For an application that has a proper configuration of AS, they would get the full RBI file generated.

Implementation

Mimic the behaviour of running initializers and eager loading of engines in classic mode and in zeitwerk mode

Tests

Made the Zeitwerk mode test explicit and added a Classic mode test.

@paracycle paracycle requested a review from a team as a code owner February 9, 2023 01:28
@paracycle paracycle added the enhancement New feature or request label Feb 9, 2023
Copy link
Contributor

@egiurleo egiurleo left a comment

Choose a reason for hiding this comment

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

This is so exciting! I mostly have questions and one small comment fix 🕺🏻

spec/tapioca/cli/gem_spec.rb Outdated Show resolved Hide resolved
lib/tapioca/loaders/loader.rb Show resolved Hide resolved
lib/tapioca/loaders/loader.rb Show resolved Hide resolved
lib/tapioca/loaders/loader.rb Show resolved Hide resolved
Copy link
Contributor

@egiurleo egiurleo left a comment

Choose a reason for hiding this comment

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

🎉

@paracycle paracycle merged commit 4cb7912 into main Feb 9, 2023
@paracycle paracycle deleted the uk-better-engine-loading branch February 9, 2023 20:59
@shopify-shipit shopify-shipit bot temporarily deployed to production February 21, 2023 12:01 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.

ActiveStorage macro methods failing type checks
2 participants