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

Migrate ViewComponent::Base.format to return a format instead of the variant #1973

Open
BlakeWilliams opened this issue Jan 12, 2024 · 4 comments
Labels

Comments

@BlakeWilliams
Copy link
Contributor

This is related to the HTML escaping conversations, but we should use the format method to correctly indicate the type of content that the component is generating, especially since that may be utilized by rails/rails#50623.

This is likely a breaking change, but I propose that we:

  1. Stop using variant as the format return value
  2. Implement some reasonable defaults, e.g. default to html for .html.erb, JSON for .jbuilder, etc.
  3. Document format as being overridable when necessary
  4. When implementing the call method without also implementing format, warn that we will default to HTML and escape.
@camertron
Copy link
Contributor

camertron commented Jan 12, 2024

This makes absolute sense to me, let's do it. /cc @jcoyne

@seanpdoyle
Copy link
Contributor

👋 Hello, author of rails/rails#50623 here.

In that PR's current state, I don't think there's any change to the significance of the #render_in object defining a #format method. That method pre-dated that PR, and was actually an implicit requirement of the interface. rails/rails#50622 documented that requirement to be explicit, and also made it optional.

@BlakeWilliams
Copy link
Contributor Author

@seanpdoyle 👋 thanks for commenting here too and sharing context!

I imagine we still want to make this change since the current behavior isn't exactly correct, and we'll get some benefits from implementing it. Right now I don't think format is reliable, and I don't imagine it gets much use outside of framework internals so I feel like it's a very small breaking change either way.

@BlakeWilliams BlakeWilliams mentioned this issue Jan 12, 2024
7 tasks
@reeganviljoen
Copy link
Collaborator

@BlakeWilliams @seanpdoyle I believe the request.format method is a more reliable way to get the format. It doesn't Make much sense to me to use a json format for example in an html request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants