qwik icon indicating copy to clipboard operation
qwik copied to clipboard

qwik-city: page routes & page endpoints don't operate correctly, and also should work like / double as data endpoints w/ pages

Open jordanw66 opened this issue 2 years ago • 6 comments

Qwik Version

latest

Operating System (or Browser)

browser

Node Version (if applicable)

No response

Which component is affected?

Qwik City

Current Behavior

Currently in production (npm run preview) page routes will generate a page when navigated to directly or via SPA. When implementing methods inside the page route (onGet, onPost, onPut, onDelete, etc.) it can double as a data endpoint and you can useEndpoint() in the page itself to fetch from these endpoints which will differ depending on client or server. On the client useEndpoint uses a fetch() to the page route itself for JSON data much like you would to a data endpoint.

However, all data requests to page routes (accept: application/json) automatically return extra JSON in the form of:

{
  isStatic: boolean,
  prefetch: string[],
  body: <JSON returned from implemented handlers>
}

This is returned on ANY requests ( w/ accept: application/json) even to HTTP methods that aren't implemented, just without the body field, along with a 200 OK status code; whereas data endpoints return a 405 Method Not Allowed.

I am not entirely sure what the isStatic and prefetch fields are for but I assume it's SPA or component related or something? However, the useEndpoint functionality when making a client-side fetch() it prepares the URL as location.href + '/q-data.json', this could be useful below?

Another issue is pages will also return 200 OK as HTML when requested with any HTTP method (without accept: application/json), such as POST or with CURL for methods that aren't browser navigable (PUT, DELETE, etc), even when those other HTTP methods aren't implemented.

Proposed Behavior

  • Pages should act like data endpoints when accept: application/json is part of the request headers and return data as returned by handler implementations directly without extra stuff.
  • Requests to pageRoute + '/q-data.json' could maintain the current behavior with extra isStatic and prefetch as needed. Same for all HTTP methods? TBD
  • All data requests to methods on page endpoints (accept: application/json) with HTTP methods that aren't implemented or valid should return 405 Method Not Allowed and either an error page or error JSON (or nothing). Along with an allow: <METHODS> header.
  • Pages requested WITHOUT accept: application/json should ALSO return a 405 w/ an error page instead when requested with an unimplemented method (like POST, DELETE, etc) or invalid methods, and allow: <METHODS> header instead. (this is how www.google.com works as an example)
  • Methods that aren't browser navigable (DELETE, PUT, PATCH, etc.) that ARE implemented but requested WITHOUT accept: application/json should be thought about how to handle. Return JSON anyway? or potentially you could return HTML on these for usecases outside direct browser navigation, but it might not be the same HTML you would want to return via GET or POST. TBD?

Usecase Example

Simple usecase for using page routes as data endpoints. You have a page route /products which produces a page that lists products and you can click-through to view product details on a page route /products/[skuId]. Both of these page routes would have internal endpoints to fetch their respective data, however delete buttons on the /products page would send a DELETE request to /products/[skuId] which would implement the onDelete handler and return JSON data indicating success or failure, etc. As a further example, another page could GET /products or /products/[skuId] for JSON data to display some data on a product elsewhere, etc.

Actual Behaviour

n/a

Additional Information

No response

jordanw66 avatar Nov 07 '22 10:11 jordanw66

I think named endpoints, something like useEndpoint('/products') would solve a lot of the data fetching problems. To your delete example, I agree that using the endpoints is clunky. If I'm on a product page and click a delete button that fetches product/[skuid] with an onDelete method Im going to get a 404 on the client regardless of how I've handled it in the route file.

ramsaylanier avatar Nov 07 '22 13:11 ramsaylanier

I think named endpoints, something like useEndpoint('/products') would solve a lot of the data fetching problems. To your delete example, I agree that using the endpoints is clunky. If I'm on a product page and click a delete button that fetches product/[skuid] with an onDelete method Im going to get a 404 on the client regardless of how I've handled it in the route file.

That's just the thing, if you're on a product page and you click a delete button that sends a DELETE method request to products/[skuId], you will not get a 404 even if you haven't implemented onDelete. Instead, you will get a 200 OK with the same HTML as if you sent a GET request. And if you send that DELETE request as a data request with the Accept: application/json header, you will also get a 200 OK response with JSON containing isStatic and prefetch[]. As I explained all above. The correct response to receiving this request without implementing onDelete or the generic catch-all onRequest is to return a 405 Method Not Allowed with an error and with an Allow: <METHODS> header.

If you do implement the HTTP method, that DELETE fetch with accept: application/json should act as a data endpoint only and return the direct JSON data from the onDelete handler indicating status response, or however you implemented it. Not the other information regarding prefetchs, which I believe should be specifically under route + '/q-data.json' as it currently also is.

(I should mention this is on prod mode with npm run preview, most of this is still the same in dev but not all)

jordanw66 avatar Nov 07 '22 17:11 jordanw66

My experience is that you'll get a 404 on the client. Maybe I'm mis-understanding the issue, but here is a reproduction as I see it:

https://stackblitz.com/edit/qwik-starter-bgt8fn?file=src/routes/page/[slug]/index.tsx

ramsaylanier avatar Nov 08 '22 01:11 ramsaylanier

My experience is that you'll get a 404 on the client. Maybe I'm mis-understanding the issue, but here is a reproduction as I see it:

https://stackblitz.com/edit/qwik-starter-bgt8fn?file=src/routes/page/[slug]/index.tsx

If you look at the network logs in dev tools you will clearly see that both buttons return a 404 to the client, even on the implemented handler. If you console.log the XHR response on the client side you will see there's nothing. So it's not even working right, but for other reasons. If you add accept: application/json to the request headers, you will see they both return 200 OK and extra JSON even in your stackblitz, regardless of whether it's the DELETE or POST method.

jordanw66 avatar Nov 08 '22 04:11 jordanw66

My experience is that you'll get a 404 on the client. Maybe I'm mis-understanding the issue, but here is a reproduction as I see it:

https://stackblitz.com/edit/qwik-starter-bgt8fn?file=src/routes/page/[slug]/index.tsx

As a followup, the reason the xhr requests are returning 404 on both the implemented DELETE method and unimplemented POST method is because it's in dev mode for some reason. Even GET to / returns 404 when I hit it with CURL. When you run an app in prod mode with npm run preview or one of the adapters like cloudflare, ALL requests regardless of method and whether it's implemented will in fact return 200 OK.

Also, even in dev mode, accept: application/json will still return 200 OK to any method, again with extra JSON. See example here: https://stackblitz.com/edit/qwik-starter-akmbjb?file=src/routes/page/[slug]/index.tsx (note in prod mode there is an extra prefetch[] included along with isStatic)

jordanw66 avatar Nov 08 '22 04:11 jordanw66

Oh yeah, duh, I forgot the headers and see what you're saying. I've never noticed this issue because I setup endpoint routes.

ramsaylanier avatar Nov 08 '22 07:11 ramsaylanier