Skip to content

feat: migrate force_json_middleware to a named middleware #8

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kerwanp
Copy link

@kerwanp kerwanp commented May 19, 2025

πŸ”— Linked issue

FriendsOfAdonis/FriendsOfAdonis#48

❓ Type of change

  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This pull-request removes the ForceJsonResponseMiddleware from the server middlewares and add it to the named middleware. The start/routes.ts is updated accordingly by providing an initial api group that uses this middleware.

This allows library authors to perform content-negotiation without requiring users to update their existing codebase. And this is a main issue for @foadonis/openapi where the content returned depends on the Accept header allowing them to use the same route for documentation UI and in tools like openapi-typescript.


On a side not I think this middleware should not exist at all. Libraries should instead rely on content negotiation as it is already something managed by browsers.

For example, here is the default Accept header on Chrome

  • document request: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7
  • fetch request: */*

With the following example, Accept is html on document request and json on fetch request.

router.get('/', ({ request }) => {
  const accept = request.accepts(['json', 'html'])
})

Also users should always define their accept header when making api request but this is a different story.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly. (n/a)

@Julien-R44
Copy link
Member

Julien-R44 commented May 19, 2025

Also agree its probably safe to remove that middleware and just let content-negotiation do its thing

+1 for deleting it ( instead of having a named middleware ) => i've also seen people having issues migrating from the API Starter Kit to Inertia and getting confused when things don't work, and this middleware was one of the reasons why

@Julien-R44 Julien-R44 requested a review from thetutlage May 19, 2025 15:09
@thetutlage
Copy link
Member

I think there are many users who use Ajax or clients like Postman and never send the relevant Accept header and then wonder why the responses from the server are not in JSON.

Also, the request.accepts(['json', 'html']) relies on the ordering of the method call and not the content negotiation at all. Which shows that if you switch the order of arguments, then the fetch calls will also start receiving HTML instead of JSON.

Therefore, completely removing the middleware might not be ideal and in this case it's a matter of choosing a default.

  • The default could be, we will force all responses to return JSON (as it is right now)
  • We will rely on content-negotiation and once you run into issues by not sending the Accept header, then you can decide to either fix the client or force it on the server.

I am okay with both the approaches. In 2nd approach, the middleware should exist but should not be registered by default.

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.

3 participants