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

Demo Site: don't exclude middleware for /api/... requests #3086

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

nsams
Copy link
Member

@nsams nsams commented Jan 12, 2025

alternative to vivid-planet/comet-starter#622

This PR moves the api routes from src/app/api into src/app/[domain]/api.

  • The api requests are technically located under a specific domain (host) like any other page, so it is not wrong to have a domain param
  • the currently implemented api routes don't use the domain param though (other apis might need it)
  • this solves errors rendering not-found in /api/..., as the not-found page accesses the domain param (main motivation for this change)

- The api requests are technically located under a specific domain (host) like any other page, so it is not wrong to have a domain param
- the currently implemented api routes don't use the domain param though (other added apis might tough)
- this solves errors rendering not-found in /api/..., as the not-found page accesses the domain param
@fraxachun
Copy link
Contributor

How does this work with the preview domain?

@nsams
Copy link
Member Author

nsams commented Jan 14, 2025

How does this work with the preview domain?

it doesn't.

Ideas to fix this:

any other ideas?

Is it generally a better idea than your fix?

@fraxachun
Copy link
Contributor

fraxachun commented Jan 14, 2025

Is it generally a better idea than your fix?

Yes, I think so.

What about:

  • Moving /api/site-preview to just /site-preview (just like we also have /block-preview at the same level) and handle it in the same way as the block-preview?
  • Handle /api/status also in a middleware?

So we get rid of the /api folder.

I could make a PR for that.

@nsams
Copy link
Member Author

nsams commented Jan 14, 2025

Moving /api/site-preview to just /site-preview (just like we also have /block-preview at the same level) and handle it in the same way as the block-preview?

yes, that would work - and the /api in the url is not really neccasary. I'll do that in this PR.

So we get rid of the /api folder.

I would not try to do that, having different /api routes is quite common. (for example in "a recent big customer project version 7" we have 6 api routes (+status and site-preview))

@nsams nsams force-pushed the middleware-dont-exclude-api branch from bb62c04 to 427d233 Compare January 14, 2025 10:34
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@fraxachun
Copy link
Contributor

So we get rid of the /api folder.

I would not try to do that, having different /api routes is quite common. (for example in "a recent big customer project version 7" we have 6 api routes (+status and site-preview))

Next.js suggest having api folders co-located to the page it is used (https://nextjs.org/docs/app/building-your-application/routing/route-handlers), this has several advantages (see also #2987). So I'm not convinced keeping /api as best practice.

@nsams
Copy link
Member Author

nsams commented Jan 14, 2025

while I fully support co-located api, this is especially for our use cases often not possible:

  • if the api is used inside something that is under the [[...path]] page
  • if the api is used at multiple places
  • if the api is used in a layout (eg for loading the menu)

@fraxachun
Copy link
Contributor

fraxachun commented Jan 14, 2025

while I fully support co-located api, this is especially for our use cases often not possible:

Convinced. I'll update vivid-planet/comet-starter#622 according to this PR.

@johnnyomair johnnyomair merged commit dab3213 into main Jan 14, 2025
12 of 13 checks passed
@johnnyomair johnnyomair deleted the middleware-dont-exclude-api branch January 14, 2025 14:03
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