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

Fix "LoadError: cannot load such file" everywhere - Remove dynamic requiring for API methods #323

Open
rusikf opened this issue Apr 25, 2024 · 1 comment

Comments

@rusikf
Copy link

rusikf commented Apr 25, 2024

Hi Hubspot Team!

On calling most API methods with invalid params We don't have validation because of dynamic requiring files
Idea 💡 - Can we remove the dynamic code required?

The ability to see the raw backtrace/logs/validation errors is much better than LoadError

Note: Everything works if pass valid parameters.

hubspot_client.crm.associations.v4.basic_api.create(object_type: 'CONTACT', object_id: nil)
# LoadError: cannot load such file -- hubspot/codegen/crm/associations/v4/models/object_id
hubspot_client.crm.associations.v4.basic_api.create
# LoadError: cannot load such file -- hubspot/codegen/crm/associations/v4/models/object_type
hubspot_client.crm.contacts.basic_api.update
# LoadError: cannot load such file -- hubspot/codegen/crm/contacts/models/contact_id

Thanks!

@rusikf rusikf changed the title Fix LoadError: cannot load such file everywhere - Remove dynamic requiring for API methods Fix "LoadError: cannot load such file" everywhere - Remove dynamic requiring for API methods Apr 25, 2024
@Samsinite
Copy link

Samsinite commented May 22, 2024

I agree. I also want to point out that the meta programming done in this library has multiple issues, including the one mentioned:

  • Dynamically loads files on method calls, as the issue author mentioned. Loading the codegen library could be restructured so that it "just works" using zeitwerk, this is the defacto ruby community standard for code loading and is highly efficient and configurable, as long as the library sets up standard conventions to use.
  • Dynamically requires and loads class names based off of hash parameter names. So if the wrong parameter name is used (which is easy to do due to the lack of documentation), the caller gets another cryptic load error. Example:
Screenshot 2024-05-22 at 1 06 05 PM
  • A big time lack of documentation, would be nice to be able to figure out how to use this library without navigating the source code of this project (which isn't the easiest to do due to all of the dynamic code generation and delegation). Tools like YARD allow you to write your own DSL to generate documentation. With some conventions and a better standard interface for defining the code generation, it seems fairly straight forward to generate this.
  • Dynamically regenerates methods on class instance instantiation, even if the method was already defined previously due to prior instantiation. This is due to these methods being defined inside of the initialization call. I'm not sure why these methods need to be redefined with the same interface every time the wrapping container class is initialized. One solution is to restructure this library to use a class method to define the classes and modules:
module Hubspot
  module Discovery
    module BaseModuleClient
      attr_reader :params

      # Defining `initialize` in a module to be included also feels like a
      # code smell, at this point why not just use a base class to
      # inherit from?
      def initialize(params)
        @params = params
      end

      class << self
        def included(base)
          base.extend(ClassMethods)
        end
      end

      module ClassMethods
        def api_modules(*module_names)
          module_names.each do |module_name|
            # Call code that create methods here
          end
        end

        def api_classes(*class_names)
          class_names.each do |module_name|
            # Call code that create methods here
          end
        end
      end
    end
  end
end

Then instead of something like:

def api_classes
  %i[
    pipeline_stages
    pipeline_stage_audits
    pipelines
    pipeline_audits
  ].freeze
end

It becomes:

api_classes :pipeline_stages,
            :pipeline_stage_audits,
            :pipelines,
            :pipeline_audits

I'm sure other solutions also exist.

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

No branches or pull requests

2 participants