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

register a CableReady JSON MIME type #140

Merged
merged 3 commits into from
Jul 17, 2021

Conversation

existentialmutt
Copy link
Contributor

@existentialmutt existentialmutt commented Jul 4, 2021

Based on conversations I was having with @leastbad yesterday, it sounds like establishing a dedicated MIME type for CableReady JSON responses delivered via cable_car could have a lot of benefits. This PR registers the application/vnd.cable-ready.json MIME type as a cable_ready format within rails.

Generally speaking, it facilitates architecture patterns I've seen @hopsoft advocating of using one controller to deliver multiple formats. A dedicated content type will allow the same controller action to serve both cable_ready or traditional JSON API requests:

respond_to do |format|
  format.cable_ready { render operations: cable_car ... }
  format.json

More specifically, registering this content type within the cable_ready gem will facilitate some changes I'm proposing to the mrujs cable car plugin to have it use MIME type as the signal of whether to run CableReady operations. See the PR for more details.

Finally, if you want to see how all this stuff fits together, I've wired up a cablecar-based modal library I'm working on to use the MIME-type based cablecar mrujs plugin. Making these changes allows that code to focus on it's own domain and offload all operation fetching and processing to the mrujs cablecar plugin and CableReady's operation serializer.

Edit

In response to some feedback from @leastbad I've added a modification to render operations: to have it send the new content type by default.

@leastbad
Copy link
Contributor

leastbad commented Jul 5, 2021

Maybe something like...

initializer "renderer" do
  ActiveSupport.on_load(:action_controller) do
    ActionController::Renderers.add :operations do |operations, options|
      render json: operations.dispatch
    end

    ActionController::Renderers.add :cable_ready do |operations, options|
      response.content_type ||= Mime[:cable_ready]
      render json: operations.dispatch
    end

    Mime::Type.register "application/vnd.cable-ready.json", :cable_ready
  end
end

Edit

Nope, this won't work. Move along, nothing to see, here.

@existentialmutt
Copy link
Contributor Author

I ran a bunch of tests on this last night and the results were pretty encouraging. Changing the default response content type for render operations: doesn’t change existing behavior because browsers are pretty forgiving when request accept and response content type don’t match.

Details in the mrujs PR:

KonnorRogers/mrujs#64 (comment)

@leastbad leastbad added enhancement proposal ruby Pull requests that update Ruby code labels Jul 6, 2021
@leastbad leastbad added this to the 5.0 milestone Jul 6, 2021
@leastbad leastbad requested a review from marcoroth July 6, 2021 00:19
@leastbad
Copy link
Contributor

leastbad commented Jul 6, 2021

LGTM!

Copy link
Member

@marcoroth marcoroth left a comment

Choose a reason for hiding this comment

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

Would it make sense to register both MIME-Types?

Mime::Type.register "application/vnd.cable-ready.json", :cable_ready
Mime::Type.register "application/vnd.cable-car.json", :cable_car

@existentialmutt
Copy link
Contributor Author

thanks for the thought @marcoroth!

Functionally any CR operations that care about MIME types are going to be using cable_car under the hood, so I'm not
sure the value having two MIME types would actually provide.

I think establishing promoting a single MIME type is best for keeping things clear. The cool thing about cable_car is that it decouples "CableReady JSON (the format)" from "CableReady (the websocket broadcaster)." CableReady is already a term that's more well known than cable_car, and I think it's better to leverage that familiarity rather than elevating a competing term.

@leastbad
Copy link
Contributor

I'm really glad @existentialmutt addressed this!

I am very convinced that cable_car should be a server-side concept. CableReady.perform() accepts CableReady JSON. It doesn't care how the sausage got made. Otherwise, I think we run the serious risk of confusing things.

Elephant in the room: I've already broken my own "rule" with the CableCar plugin for mrujs. The reasons are:

  • the CableReady constant is already in play when declaring the plugin, so it has to be called something different
  • mrujs CableCar plugin is designed to talk to end points that return JSON generated with cable_car, sooo...

Do as I say, not as I do! 😜

@marcoroth
Copy link
Member

The cool thing about cable_car is that it decouples "CableReady JSON (the format)" from "CableReady (the websocket broadcaster)."

That's the thing. We are now using the term of the websocket broadcaster (CableReady) for the JSON format respectively the MIME-Type.

That's why I thought that it could make sense to register both so that you don't chose the wrong one because both work 😉

@leastbad
Copy link
Contributor

When you put it that way, seems harmless enough and potentially even useful.

@existentialmutt
Copy link
Contributor Author

Fair enough @marcoroth . What I really care about is having the library promote a standard MIME type(s), so I can get behind a second cable-car type.

@leastbad leastbad merged commit e6f502e into stimulusreflex:master Jul 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement proposal ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants