Open liveh2o opened 2 years ago
I found this comment discussion money-patching in Zeitwerk: https://github.com/fxn/zeitwerk/issues/141#issuecomment-724618609
Are we ok to keep things the same if eager loading is used all the time? Just trying to figure out how broken this will be with Rails 7.
This issue isn't fixed by eager loading because it's more of an autoloading issue. Because these constants (modules, classes) are already defined by our Protobuf definition gem, they are skipped by Zeitwerk.
Otherwise, this horrible hack is needed in config/application.rb
:
services = "#{Rails.root}/app/services"
Rails.autoloaders.main.ignore(services)
config.to_prepare do
Dir.glob("#{atlas_services}/atlas/bifrost/*_service.rb").each do |service|
load service
end
end
With the middleware pattern introduced a while back, adding a routing layer shouldn't be terribly difficult (famous last words, I know).
Is there a way for us to write a simple routing (or loading) layer without breaking the interface?
I'm pretty sure there is since the "interface" is simply reopening the class to monkeypatch it. The final middleware in the stack is the service dispatcher. It's responsible for calling the endpoint, and capturing the response.
Seems like adding a router there with some sensible defaults/fallbacks is the simplest approach.
The workaround, for now, is to simply use the classic autoloader. That works with Rails 6.x. I planned on tackling this once we have our internal apps all running Rails 6.1, which is getting closer.
Adding few cents: actually because of strange decision to suffix all files with .pb.rb
besides making filenames non-standard, the generated code is not compatible with ZW at all:
loader = Zeitwerk::Loader.new
loader.push_dir(Pathname(__dir__).join('pb'))
loader.setup
loader.eager_load
Zeitwerk::NameError: wrong constant name Common.pb inferred by Zeitwerk::Inflector from file (Zeitwerk::NameError)
This affects any other way to autoload, but native ruby, where you must set path for each const. It's worth to say that native Google's compiler makes similar thing, by adding _pb
suffix, but at least the result is ready to be autoloaded.
Am I miss something?
Thanks!
Offtopic: is ruby_package
option respected somehow? I can't see it really does what is it for. As mentioned in #238 if definitions are used in many langs this top namespacing is a must, to avoid noisy definitions and improve portability. For example there won't be a need in any suffixes above, if we can just put everything generated in top level module of choice.
Ah, I hadn't even considered that the suffix pattern would be a problem for Zeitwerk as well. Seems like the _pb
pattern might work better.
As for the question on ruby_package
, @qd3v, can you open an issue so we can discuss?
Rails 6.0 has a new autoloading mode which delegates to the Zeitwerk gem:
Our RPC service class implementation depends on a stub being generated from the protobuf definition, then being reopened in your application (i.e., monkey patching) to implement the service endpoints:
This pattern is incompatible with the Zeitwerk autoloader because files are loaded as classes/namespaces are defined by the generated Ruby protobuf definition. When Zeitwerk encounters the namespace/class while loading
app/services
(a sensible place to put service classes in a Rails app), the files are skipped:To fix this, we need to reconsider how services are expected to be implemented.
One option would be to treat the generated service as a router that dispatches RPC requests to a separate application service class similar to the Rails router. The first cut would simply create the glue that allows this routing to take place based on the generated services (i.e., endpoint names and class methods would still need to be the same; no custom routing would be allowed). A future cut could make this more flexible, allowing requests to be routed as needed (i.e., conditionally based on a version scheme, etc.), but that may not be needed.
I would also anticipate this change being part of a major release in the event that backwards incompatible changes are required.