Skip to content

[Rendering] Custom renderer support#244

Open
anuj-pal27 wants to merge 5 commits intorage-rb:mainfrom
anuj-pal27:feat/custom-renderers-234
Open

[Rendering] Custom renderer support#244
anuj-pal27 wants to merge 5 commits intorage-rb:mainfrom
anuj-pal27:feat/custom-renderers-234

Conversation

@anuj-pal27
Copy link
Contributor

@anuj-pal27 anuj-pal27 commented Mar 20, 2026

Summary

Implements the config.renderer(:name) { ... } DSL proposed in the issue.
Registers custom renderer blocks and generates render_<name> methods on
RageController::API at boot time.

Key behavior

  • config.renderer(:csv) { |obj, **opts| ... } registers a renderer block
  • After __finalize, every controller automatically gets render_csv, render_phlex, etc.
  • Generated methods support status: as both Symbol (:created) and Integer (201)
  • Unknown status symbols raise ArgumentError instead of setting nil status
  • Renderer blocks run in controller context — headers, request, params all work
  • Double-render is prevented whether render happens via return value or internal render call
  • RageController::API.__custom_renderers registry makes finalize idempotent across re-runs

Tests

9 specs covering:

  • Basic registration and method generation
  • Status handling (symbol, integer, default 200)
  • Duplicate renderer registration guard
  • Method name conflict detection
  • Controller context access (headers, params)
  • nil return value → empty string body
  • Renderer block calling render internally
  • Double-render protection

Notes

Renderer blocks are converted to named methods via define_dynamic_method rather
than using instance_exec per request — this keeps the hot path allocation-free
and consistent with how Rage handles action methods internally.

Fixes: #234

Screenshots

CSV renderer:
Screenshot from 2026-03-21 00-03-07

Phlex renderer:
Screenshot from 2026-03-21 00-06-09

Copy link
Member

@rsamoilov rsamoilov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @anuj-pal27

That's a great start! I've left some comments. Please let me know if you have any questions.

@anuj-pal27
Copy link
Contributor Author

hii @rsamoilov I have one question that I updated render plain: to use @__headers["content-type"] ||= "text/plain; charset=utf-8" so custom renderers can set their own content-type (like text/csv) without it being overwritten when delegating to render. But there's a problem — RageController::API#initialize already sets the default content-type to application/json, so ||= ends up keeping JSON instead of switching to text/plain, which breaks existing specs. Should render plain: always force text/plain, or should it keep the existing content-type and I update the specs?

@rsamoilov
Copy link
Member

But there's a problem — RageController::API#initialize already sets the default content-type to application/json, so ||= ends up keeping JSON instead of switching to text/plain

Exactly! Which is why you would need to assign the default application/json content type to a constant. And then you would update the condition in the plain branch to set the content type only if it's the default one.

@anuj-pal27
Copy link
Contributor Author

@rsamoilov , Done — changed __define_custom_renderers to iterate with @renderers.each since _finalize already guards if @Renderers.
Also removed the explicit double-render raise from the generated render
method and rely on render for raising.
For the content-type behavior: introduced DEFAULT_CONTENT_TYPE (JSON) and now render plain: only switches to text/plain when the current content-type is nil or equals the default JSON; custom types like text/csv are preserved.

Thank you for the clear pointers/review — they helped a lot.

end # class << self

DEFAULT_CONTENT_TYPE = "application/json; charset=utf-8"
DEFAULT_PLAIN_CONTENT_TYPE = "text/plain; charset=utf-8"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this value as a constant.

Suggested change
DEFAULT_PLAIN_CONTENT_TYPE = "text/plain; charset=utf-8"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that — you’re right. I’ll remove DEFAULT_PLAIN_CONTENT_TYPE and keep only the default JSON content-type as a constant, and update the plain branch to override only when it’s still the default.

else
@__headers["content-type"] = "text/plain; charset=utf-8"
ct = @__headers["content-type"]
@__headers["content-type"] = DEFAULT_PLAIN_CONTENT_TYPE if ct.nil? || ct == DEFAULT_CONTENT_TYPE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@__headers["content-type"] = DEFAULT_PLAIN_CONTENT_TYPE if ct.nil? || ct == DEFAULT_CONTENT_TYPE
@__headers["content-type"] = "text/plain; charset=utf-8" if ct.nil? || ct == DEFAULT_CONTENT_TYPE

end

RageController::API.class_eval <<~RUBY
def render_#{name}(*args, status: nil, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really sorry, I didn't realise that return if @__rendered will prevent the code from going into render and raising the exception. We actually need it, could you please add it back? Sorry again.

Suggested change
def render_#{name}(*args, status: nil, **kwargs)
def render_#{name}(*args, status: nil, **kwargs)
raise "Render was called multiple times in this action." if @__rendered

Copy link
Contributor Author

@anuj-pal27 anuj-pal27 Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem — I’ll add the raise Render was called multiple times in this action.if @__rendered back at the start of render_#{name} (and keep return if @__rendered after the block).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

Thinking about responsibilities, there's one more thing I'd love to improve. Currently, the configuration class defines the custom renderer methods on RageController::API. Ideally though, we want the configuration class to simply hold the data and know as little about the outside world as possible.

  • Let's update the renderer method to not call Rage::Internal.define_dynamic_method - all it should do is just store the block in a RendererEntry
  • In __define_custom_renderers, let's move everything inside the RageController::API.method_defined? condition into a dedicated method in RageController::API, e.g. __register_rederer

This way, Configuration won't know the internal details of RageController::API, and the loop inside __define_custom_renderers will simply iterate over the renderers, call __register_rederer and flip the applied flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good — I’ll refactor so Configuration#renderer only stores the block in RendererEntry, and move the method-definition/conflict-check logic into RageController::API.__register_renderer. Then __define_custom_renderers will just iterate, call __register_renderer, and mark the entry as applied.

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

Successfully merging this pull request may close these issues.

[Rendering] Custom renderer support

2 participants