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

Fix Zeitwerk loading EmailAlertAPI constant #1346

Merged
merged 1 commit into from
Jul 30, 2020
Merged

Conversation

kevindew
Copy link
Member

This resolves a problem where the Email Alert API application was not
starting when using the Zeitwerk autoloader. The application wasn't
starting because it couldn't find the EmailAlertAPI constant and was
triggering this error:

/Users/kevindew/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/zeitwerk-2.4.0/lib/zeitwerk/loader.rb:392:in `const_get': uninitialized constant EmailAlertApi::Config (NameError)

The autoloader would expect to find a class named EmailAlertApi rather
than EmailAlertAPI at the location of email_alert_api.

@richardTowers
Copy link
Contributor

Do we not have any tests that cover the autoloader?

@kevindew
Copy link
Member Author

kevindew commented Jul 30, 2020

Do we not have any tests that cover the autoloader?

We can get caught out with the differences in loading behaviour between test environments and production. When production starts it eager loads all the classes where this error can occur. Whereas in test we avoid eager loading the whole application and as this constant gets loaded by something else:

require_relative "../lib/email_alert_api/config"

(which I'm going to see if I can now remove with this change - sadly still needed)

This situation is actually one of the ones that the publishing-e2e-tests suite can catch as it runs the application in a production environment, which makes it good for catching the parts of code that the test suite doesn't exercise but are essential for basic functionality.

Copy link
Contributor

@richardTowers richardTowers left a comment

Choose a reason for hiding this comment

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

Fair enough!

I'd assumed that the feature tests would cover this (as they run the entry point of the application), but I guess they don't because config.eager_load is false in all test environments, even feature tests.

Feels like it would be nice for the test suite of the app to cover "won't even start in production", but if that's not easy to do I'm happy to merge this with no regression test.

@kevindew
Copy link
Member Author

Thanks @richardTowers, I was busy editing my earlier reply as I learnt more from digging further. It's probably easier to just write a fresh reply (and a reminder that I should probably slow down 😅 )

So the issue wasn't actually that it fails to autoload the class we want, it's actually that when the new Zeitwerk autoloader runs it expects to find a constant with the name EmailAlertApi::Config at the path it loaded and errors when one isn't defined. So rather this actually being a problem with it not finding the class we want, it instead errors for this different reason.

There's a couple of smells about this to look into further. This EmailAlertAPI::Config is a somewhat odd pattern, which we should see about removing. It also might not be too big a deal to eager_load = true in tests, particularly with trying out spring, since in my quick tests it doesn't seem to make much time difference - however this would be unconventional for a Rails app.

This resolves a problem where the Email Alert API application was not
starting when using the Zeitwerk autoloader. The application wasn't
starting because it expected that the `lib/email_alert_api/config`
directory would define a constant EmailAlertApi::Config and when it
didn't it raised an error.

```
/Users/kevindew/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/zeitwerk-2.4.0/lib/zeitwerk/loader.rb:392:in `const_get': uninitialized constant EmailAlertApi::Config (NameError)
```

This was a problem that only affects the production environment because
this is the only environment that has config.eager_load set to true. In
other environments classes are autoloaded when needed.
@kevindew kevindew merged commit f51e644 into master Jul 30, 2020
@kevindew kevindew deleted the fix-broken-const branch July 30, 2020 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants