procore-oss / blueprinter

Simple, Fast, and Declarative Serialization Library for Ruby
MIT License
1.13k stars 109 forks source link

Move global configuration #403

Open sandstrom opened 7 months ago

sandstrom commented 7 months ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe

If blueprinter is used for different things in a single codebase, the global configuration cannot be used.

For example, one might have two APIs (public and internal), and want to use blueprinter for both, but with different configuration for each.

Describe the feature you'd like to see implemented

Deprecate global configuration and move configuration to the base class.

Make it possible to override in subclasses, thereby relying on the common class inheritance pattern to handle "global configuration".

# One group of blueprinters
class PublicApiBlueprint < Blueprinter::Base
  # DSL alt 1
  config do
    sort_fields_by = :definition
  end

  # DSL alt 2
  config.sort_fields_by = :definition
end

class UserBlueprint < PublicApiBlueprint
  identifier :uuid

  fields :first_name, :last_name
end

# Another group of blueprinters
class InternalApiBlueprint < Blueprinter::Base
  # DSL alt 1
  config do
    sort_fields_by = :score
  end

  # DSL alt 2
  config.sort_fields_by = :score
end

class GroupBlueprint < InternalApiBlueprint
  identifier :id

  fields :name, :members_count
end

As for making sure configs are not shared (only copied) down the inheritance hiearchy, I'd probably pull in class_attribute from ActiveSupport (https://guides.rubyonrails.org/active_support_core_extensions.html#class-attributes).

If you want to add it manually there is some inspiration here:

Describe alternatives you've considered

No response

Additional context

No response

sandstrom commented 7 months ago

Happy to discuss this in more detail, and elaborate more.

lessthanjacob commented 7 months ago

@sandstrom Apologies for the delay here, and thanks for the detailed suggestion!

I think this would be a valuable addition to the library, and leveraging class_attribute for part of the implementation seems reasonable to me!

ritikesh commented 6 months ago

class_attribute is a rails implementation and historically, we've shied away from doing rails specific implementations. We can however, use class instance variables.

lessthanjacob commented 6 months ago

class_attribute is a rails implementation and historically, we've shied away from doing rails specific implementations. We can however, use class instance variables.

Not to split hairs here, but I wouldn't consider it explicitly a Rails implementation; this functionality was extracted from rails and is encapsulated entirely within an activesupport gem which can technically be used in any Ruby project.

If we don't want to bring in an intentional dependency on activesupport due to the side-effects it has on Ruby core classes, then I think that's a reasonable argument. The library is available to us currently due to our dependency on activerecord (which is partly why I wouldn't necessary be opposed to using it here), but truth be told I'd also like to fully remove that dependency as well.

sandstrom commented 6 months ago

I don't have a strong preference.

Should be fairly easy to replicate the logic of class_attribute from Rails, using class instance variables and the inherited method.

Breaking changes

What's your view on breaking changes and major releases? Some projects are happy about frequent major release, others prefer making them only once every year or two.

I'm fine with either, but it's good to know since this change could potentially be implemented as a breaking change, but also doable in a manner where global conf is used as fallback (non-breaking scenario).

ritikesh commented 6 months ago

This is a major refactor and I would personally prefer combining this with any/some other major deprecations planned for the near future as a potential 2.x release. Maybe this could go into a separate 2.x branch.

github-actions[bot] commented 4 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sandstrom commented 4 months ago

Still relevant!

jhollinger commented 4 months ago

I think I'm on board with this. "Global" config could go into an ApplicationBlueprint which all others could choose to inherit from or not.

I'd vote against using ActiveSupport for class_attribute. Pretty easy to accomplish the same thing using inherited.

sandstrom commented 4 months ago

@jhollinger Do you know of a blog post or similar explaining how to replicate class_attribute behavior (more or less) with inherited?

Or are you simply thinking of something like this:

class Mother
  @things = []

  class << self
    attr_accessor :things
  end  

  def self.inherited(subclass)
    puts "New subclass: #{subclass}"
    subclass.things = self.things.dup
  end
end

class Child < Mother
end
jhollinger commented 4 months ago

@sandstrom Yes, assuming we only have a handful of attributes to keep track of, the above is what I was thinking.

RazvanFarte commented 4 months ago

Hi guys,

Hope I'm on the right topic. Should this mean that after this feature is implemented, it will be possible to have two separate sort_fields_by configs, per class level?

I'm trying to work around an issue with setting this configs, that impacts the views and associations in another Blueprint. The issue happens due to this line sorted_fields = sort_by_definition ? sort_by_def(view_name, fields) : fields.values.sort_by(&:name) in fields_for. And the only fix found was to have the configure block before and after the first Blueprint definition, which looks ugly to me. Would prefer sandstorm's version. Not sure if it rings a bell, but I could open a new issue, so it could be referenced here and hopefully some configs to be available at Blueprint level, instead of global.

Here the fix I found that works for me:

Blueprinter.configure do |config|
  config.sort_fields_by = :definition
end

module CustomerApi
  class BaseBlueprint < Blueprinter::Base
  end
end

Blueprinter.configure do |config|
  config.sort_fields_by = :name_asc
end
sandstrom commented 4 months ago

Should this mean that after this feature is implemented…

Yes, that would be possible

github-actions[bot] commented 2 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sandstrom commented 2 months ago

Not stale

jhollinger commented 1 month ago

FYI, this is now part of a proposed V2 base class in #437 (along with other changes). All configuration, and fields, are held in Blueprint classes and are inheritable. To simulate "global" config in V2, options and default fields can be configured in an ApplicationBlueprint class which others inherit from.