Stop requiring stuff from DSL compilers #1765
Conversation
DSL compilers should not change the object space of the application they run in, and thus should not really be doing any requires. This commit removes all requires from DSL compilers, and instead relies on the application to require the necessary files. A DSL compiler should be responsible for checking if the constants it is expecting to have been loaded are actually loaded, and if not, it should not be defining the DSL compiler class at all.
paracycle
force-pushed
the
uk-change-dsl-activation-logic
branch
2 times, most recently
from
edda967
to
c481d0f
Compare
paracycle
force-pushed
the
uk-change-dsl-activation-logic
branch
from
c481d0f
to
9d3a84d
Compare
|
Nevermind, I just missed them somehow... 🤦♂️ |
| rescue LoadError | ||
| return | ||
| end | ||
| return unless defined?(AASM) |
There was a problem hiding this comment.
This was requiring Active Record as well. Does the compiler need ActiveRecord to be defined too?
There was a problem hiding this comment.
It doesn't. I think we were requiring AR to activate the AR support in AASM, but I don't think that's something that is required.
I'll check this again, though.
|
Would it be worth adding a cop to prevent any accidental future use of |
You actually make a great point. I need to change how extensions work, since they are loaded before the application is loaded. As things stand, these extensions just won't work. |
Extensions cannot guard against the constants being available at load time, since we load extensions before we load the application. Instead, we need to register load hooks that will be called when the relevant constants are available. For now, this only works for Active Record in every case, since Active Record has a lazy load hook that we can use. For Frozen Record and Kredis we need to rely on the Rails load hooks to delayed trigger of the extension loading. But, that won't be available outside of Rails.
paracycle
force-pushed
the
uk-change-dsl-activation-logic
branch
from
6753bfe
to
fe1dae4
Compare
|
Ok, I've reworked the extensions to do lazy loading, and made the |
paracycle
force-pushed
the
uk-change-dsl-activation-logic
branch
from
fe1dae4
to
bc161a7
Compare
|
Test failures seem legit |
Instead of refusing to load the compiler if certain constants are not defined, we can make our dependency on those constants to be dynamic instead.
paracycle
force-pushed
the
uk-change-dsl-activation-logic
branch
from
bc161a7
to
c2983e7
Compare
|
Ok, tests are fixed! |
| rescue LoadError | ||
| return | ||
| end | ||
| return unless defined?(Rails) && defined?(ActiveSupport::TestCase) && defined?(ActiveRecord::TestFixtures) |
There was a problem hiding this comment.
I wonder if we could define a defined?(*args) in self?
There was a problem hiding this comment.
I don't think that's possible since defined? is a special language construct and not a method. If we define a defined? method, then arguments to it will be evaluated at the call site, but that's not what we want.
Motivation
Fix #1759
Fix #1425
DSL compilers should not change the object space of the application they run in, and thus should not really be doing any requires. This commit removes all requires from DSL compilers, and instead relies on the application (or, in our case, the tests) to require the necessary files.
A DSL compiler should be responsible for checking if the constants it is expecting to have been loaded are actually loaded, and if not, it should not be defining the DSL compiler class at all.
Implementation
requirelines from DSL compilers, replace them with early returns guarding fordefined?for relevant constants.requirelines insidebefore_setupmethods in tests, so that they get required after we fork (for isolation), but before we load the actual DSL compiler class.:before_configurationhook to check to see ifFrozenRecordorKredishave been loaded and do the mix-in. This, however, only will work properly for Rails applications.Tests
Updates tests