-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Make Controller Helpers autoloadable #6062
base: main
Are you sure you want to change the base?
Conversation
ea50bb9
to
1eced48
Compare
I think the reason to have them in lib is to being easily loadable by host applications and extensions. Can we do the same now? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6062 +/- ##
==========================================
- Coverage 92.52% 89.39% -3.13%
==========================================
Files 385 791 +406
Lines 8400 18274 +9874
==========================================
+ Hits 7772 16336 +8564
- Misses 628 1938 +1310 ☔ View full report in Codecov by Sentry. |
Yes, they have the same loading characteristics as e.g. |
Right, we just don't need any require before using it now. What about keeping the files and print a warning telling people that they can remove the require now? I'm afraid this might break all stores requiring the lib files. |
39e835b
to
38e9282
Compare
Good catch. I've re-added the files in |
There's no real good reason to keep these files in lib and require them on app startup. Let's put them in app/ and let Zeitwerk handle their loading.
These are easy enough.
38e9282
to
cf07217
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
There's no real good reason to keep these files in
lib
and require them on app startup. Let's put them inapp/
and let Zeitwerk handle their loading.