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

fix: don't render json response if url contains /api/ #951

Merged
merged 1 commit into from
Feb 16, 2023
Merged

fix: don't render json response if url contains /api/ #951

merged 1 commit into from
Feb 16, 2023

Conversation

danielroe
Copy link
Member

πŸ”— Linked issue

nuxt/nuxt#19093

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 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

Although a helpful idea, I think this is too broad a net for rendering an API response. Even routes starting with /api might still benefit from an HTML response if requested from a browser.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroe danielroe added the bug Something isn't working label Feb 16, 2023
@danielroe danielroe requested a review from pi0 February 16, 2023 22:13
@danielroe danielroe self-assigned this Feb 16, 2023
@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Merging #951 (a666df5) into main (be1ac07) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #951   +/-   ##
=======================================
  Coverage   67.59%   67.59%           
=======================================
  Files          59       59           
  Lines        6030     6030           
  Branches      679      679           
=======================================
  Hits         4076     4076           
  Misses       1945     1945           
  Partials        9        9           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@pi0 pi0 merged commit ee565d3 into main Feb 16, 2023
@pi0 pi0 deleted the fix/api branch February 16, 2023 22:46
@dargmuesli
Copy link
Contributor

dargmuesli commented Feb 21, 2023

Isn't that a breaking change? With Nuxt requests to server/api endpoints - that are not named *.json because they can return other mimetypes in my case - now return html instead of the expected json.

@danielroe
Copy link
Member Author

danielroe commented Feb 21, 2023

~> see #969

the behaviour you mention was a bug, but one that surfaced due to this change but not caused by it. Or so I tell myself πŸ˜†

@dargmuesli
Copy link
Contributor

Thank you for the background, I see πŸ‘ Intuitively I'd expect "root" /api routes to keep the previous json behavior, not matter how they're accessed.

I have an endpoint that either returns a specific content type (text/calendar) or throws a createError. I'd expect json for errors, even when calling that endpoint by manual input into the browser's address bar.
I don't want to rename the endpoint to *.json as it can return a different mime type. I also don't want to set the Content-Type header manually before errors. I'm not sure if the cors-related fix will solve that question, but I'll try later πŸ™Œ
Is there something I should do instead? Maybe I don't see the obvious way to do it now.

Copy link
Member Author

I understand what you mean, though equally I would not expect users to access an /api/ endpoint directly in the browser and receive JSON (but only in the event of an error).

Would you open an issue to discuss adding a .startsWith() test? We might also consider allowing handlers to indicate their preference somehow, or something else.

cc: @pi0 who may have a better idea.

@pi0
Copy link
Member

pi0 commented Feb 21, 2023

Reverting in #971 for now.

@dargmuesli
Copy link
Contributor

I've created #978 as a place where a discussion can continue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants