Support Zeitwerk

[adfoster-r7]

Just copy/pasting a separate idea from #157 that I would love to have opinions on; the goal is to solve the problem of there being a global central registry in BinData for resolving class details - which has caused issues for a larger code base with lazy loading, or accidentally having duplicate definitions etc

I wonder if it'd be possible to support a more explicit method for the bindata DSL, so that instead of relying on metaprogramming for the type lookups - the user could supply the type explicitly via a model (or field, etc) method:

class Request < BinData::Record
    endian :big
    uint8 :message_type, initial_value: 3
    uint8 :incremental
-   rectangle :rectangle
+   field :rectangle, type: Rectangle
end

Since we can explicitly specify the Ruby class this has the benefit of Ruby's inbuilt namespacing support (field :rectangle, type: V1::Rectangle etc), being compatible with Zeitwerk for lazy-loading large/complex object graphs (as currently we have to eagerly load complex models upfront to ensure the types are available for the bindata DSL to work), and it also better integrates with a code editor out of the box - code completion, refactoring, navigating to definitions etc.

Choice example before/after:

-  class TightCompressionBasicCompression < BinData::Record
+  class BasicCompression < BinData::Record
     endian :big
 
-    search_prefix :tight_compression_basic_compression
-
     # Two bits dedicated to which stream to use
     #
     # 00 - Use stream 0
@@ -150,12 +149,14 @@ class RubyVnc::Decoder::Tight
     uint8 :filter_id, onlyif: -> { (compression_flag & 0b0100) >> 2 == 1 }
 
     choice :filter_value, selection: -> { filter_id } do
-      copy_filter BasicCompressionFilterType::COPY_FILTER,
-                  width: -> { width },
-                  height: -> { height }
-      palette_filter BasicCompressionFilterType::PALETTE_FILTER,
-                     width: -> { width },
-                     height: -> { height }
+      field BasicCompressionFilterType::COPY_FILTER,
+            type: CopyFilter,
+            width: -> { width },
+            height: -> { height }
+      field BasicCompressionFilterType::PALETTE_FILTER,
+            type: PaletteFilter,
+            width: -> { width },
+            height: -> { height }
     end
   end

In the above snippet we can drop the search_prefix workaround since we rely on Ruby's resolving logic for looking up the Ruby class, and we can drop the verbose TightCompressionBasicCompression class name prefix from our bindata definitions - which I had added previously to support bindata's type resolution lookup:

class RubyVnc::Decoder::Tight

-  class TightCompressionBasicCompressionPaletteSize < BinData::BasePrimitive
+  class PaletteSize < BinData::BasePrimitive
...

-  class TightCompressionBasicCompression < BinData::Record
+  class BasicCompression < BinData::Record

...

My local bindata hack for spiking this was just a branch of v4.10.0 (since that's what my older app was using at the time):

diff --git a/lib/bindata/sanitize.rb b/lib/bindata/sanitize.rb
index 7325b20..347c8fe 100644
--- a/lib/bindata/sanitize.rb
+++ b/lib/bindata/sanitize.rb
@@ -15,6 +15,9 @@ module BinData
 
       if BinData::Base === obj_type
         obj_class = obj_type
+      elsif :field == obj_type
+        # TODO: Confirm if raw_hints should be honored here
+        obj_class = obj_params[:type]
       else
         obj_class = RegisteredClasses.lookup(obj_type, raw_hints)
       end

A real implementation would likely have to honor raw_hints - and provide a way to opt out of registering with bindata's global dsl registry

I didn't run through all of the edgecases, so apologies if this approach has been discussed and ruled out as a valid approach previously! 💯

[dmendel]

There are two issues here.

• a global central registry
• lazy loading

I am currently working on the central registry problem and expect to have a solution committed in the next few days. Can you please elaborate more on the problem with lazy loading?

[adfoster-r7]

There are two issues here
...

Maybe an extra issue to add to the list: on the tooling side it's hard (out of the box) to navigate from a definition to its implementation. i.e. command clicking with an editor on the class here will navigate you to the required file:

field :rectangle, type: Rectangle

Whilst command clicking the method name here won't work in an editor:

rectangle :rectangle

Can you please elaborate more on the problem with lazy loading?

If you have two files:

class SomeRequest < BinData::Record
  rectangle :rectangle
end

class Rectangle < BinData::Record
  ...
end

With zeitwerk enabled, I can't do:

SomeRequest.new

Unless I have eager loading enabled with zeitwerk, or explicitly require the Rectangle class first so that it's registered as an option in BinData's DSL it won't work.

However, if we used this pattern of explicitly specifying the types - Zeitwerk would load the Rectangle class first and the dependency tree could continue to be lazy loaded - i.e. we don't need to upfront load all bindata files, just the critical path to load the SomeRequest

class SomeRequest < BinData::Record
  field :rectangle, type: Rectangle
end

For a larger project we've found eager loading multiple bindata definitions that aren't needed to be slow, but using lazy loading would cause issues

[dmendel]

Support for Ruby Modules inside the registry has been added.

This is detailed here: https://github.com/dmendel/bindata/wiki/AdvancedTopics#namespaces

I'll consider lazy loading shortly

[adfoster-r7]

Awesome - thanks!

What are your thoughts on the approach of leaning on Ruby's classes? 👀

class SomeRequest < BinData::Record
  field :rectangle, type: Rectangle
end

[dmendel]

Here is your example.

class Rectangle < BinData::Record
...
end

class SomeRequest < BinData::Record
  rectangle :my_rect
end

Did you know you can also write it like this?

class SomeRequest < BinData::Record
  Rectangle :my_rect
end

This is not officially supported, but it works in most cases.

[dmendel]

For a larger project we've found eager loading multiple bindata definitions that aren't needed to be slow, but using lazy loading would cause issues

By not needed, do you mean multiple definitions in separate trees, or definitions in a single tree that are selected by Choice?

[franzliedke]

Very happy to see this. 💚

Sorry I never got back with more ideas on the "Multiple registries" topic (or find the time to propose an implementation 😢). Happy to chime in if more input is desired.