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

Move app config requirement to the top #676

Merged
merged 1 commit into from
Aug 20, 2021
Merged

Move app config requirement to the top #676

merged 1 commit into from
Aug 20, 2021

Conversation

wout
Copy link
Contributor

@wout wout commented Aug 20, 2021

I was having an issue with the rosetta shard because config files and initializers were loaded after everything in /src.

Rosetta requires the locales to be loaded and parsed before the app gets loaded. This happens with the Rosetta::Backend.load("config/rosetta") macro. Since config files were loaded last, the compiler couldn't find the locale modules, which are generated from an external file using the run macro. So every single locale raised compiler errors:

Compiling...
web          | There was a problem expanding macro 't'
web          | 
web          | Code in macro 't'
web          | 
web          |  3 | Rosetta.t("sign_ins.new_page.heading")
web          |      ^
web          | Called macro defined in lib/rosetta/src/rosetta/translation.cr:6:3
web          | 
web          |  6 | macro t(key)
web          | 
web          | Which expanded to:
web          | 
web          |  > 2 | 
web          |  > 3 |     
web          |  > 4 |       Rosetta::Locales::SignIns::NewPage::Heading
web          |              ^------------------------------------------
web          | Error: undefined constant Rosetta::Locales::SignIns::NewPage::Heading

Simply moving the config requirement to the top of the list in src/app.cr fixed the issue. So far, I can't see any problems with this change and I've tested it with two apps.

I may be wrong, but to me, it makes more sense to load configuration files before loading the rest of the app. Or am I overlooking anything?

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

I just tried this in one of my apps, and it seems to work fine. 👍

I think maybe the only time it might be wonky is if you had a file in your app that required some config, but it was required after the config at this points... However, since this would only affect new apps, they could easily just add their new files first or whatever.

@jwoertink jwoertink merged commit 6dee95d into luckyframework:master Aug 20, 2021
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.

2 participants