feat(rewrite): document rewrite APIs#8914
Conversation
✅ Deploy Preview for astro-docs-2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
sarah11918
left a comment
There was a problem hiding this comment.
Just publishing a partial review (API reference and first paragraph of Routing page) just so nothing happens to it in the meantime.
Still to review: Routing page i18n example and middleware page!
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
|
@sarah11918 FYI this commit |
| The status code returned by the request will change based on the result of the rewrite. For example, if you call `Astro.rewrite("/foo/bar")`, but `/foo/bar` doesn't exist even though you have a dynamic route, Astro will return a `404` status code. | ||
|
|
||
| You can take advantage of this logic by conditionally return different status code. For example, for a certain user can't access certain routes, you can call `Astro.rewrite("/404")` and Astro will render the content of the 404 page (built-in or custom). |
There was a problem hiding this comment.
This doesn't feel very clear right now.
Is the first paragraph anything more than routes that don't exist will 404 (like any other routes? not sure why this is special?). I don't feel like this paragraph is helpful, or provides any specific information about the rewrite. So you'll have to connect the dots a little stronger for me if this is really important!
Is the second paragraph really about conditionally rewriting (not about returning status codes?) If there's a 404 page, then the 404 page itself isn't 404'ing, right? So from the user perspective using this feature, you are rewriting with a path that does exist, just like any other path? (It just happens to be a 404 page) As written, this doesn't feel like it's about status codes since it's describing rewriting to a page that exists.
Since the other examples of usage have been very simple, if you want to talk about conditional rewrites it might be nice to actually show the code for a conditional rewrite here. Using the example of 404'ing for a restricted page is a good example, but I think it's more helpful to show what that code looks like!
(And of course, whatever we decide here, we can use for the context entry below, too, or decide just to link up here!)
There was a problem hiding this comment.
Is the first paragraph anything more than routes that don't exist will 404 (like any other routes? not sure why this is special?). I don't feel like this paragraph is helpful, or provides any specific information about the rewrite. So you'll have to connect the dots a little stronger for me if this is really important!
Ok, I'll try to use some bridge phrase.
Is the second paragraph really about conditionally rewriting (not about returning status codes?)
It is not about conditional rewriting, it's about status code and expectations, but it's a bit silly to say if you do this, this other thing happen. So, I was trying to put some real usage context. Let me see if I can do something better with an example.
There was a problem hiding this comment.
@sarah11918 please have a look at this new version af382c0 (#8914)
Co-authored-by: Armand Philippot <59021693+ArmandPhilippot@users.noreply.github.com> Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
sarah11918
left a comment
There was a problem hiding this comment.
Whew! OK, here are some thoughts for the rest of the document!
See what you like, (and what is correct) and change/commit whatever makes you happy. When there are lots of comments like this, it can be distracting to read and review, so please do go ahead and commit whatever changes you want. It's easier for me to read from a fresh file again.
| The `APIContext` exposes a method called `rewrite`, which works the same way as [Astro.rewrite](/en/guides/routing/#rewrites). | ||
|
|
||
| The `next` function optionally accept a parameter that allows to **rewrite** the current `Request`. For example, if a user isn't logged in, you could **rewrite** the current request without the need of a redirect: |
There was a problem hiding this comment.
| The `APIContext` exposes a method called `rewrite`, which works the same way as [Astro.rewrite](/en/guides/routing/#rewrites). | |
| The `next` function optionally accept a parameter that allows to **rewrite** the current `Request`. For example, if a user isn't logged in, you could **rewrite** the current request without the need of a redirect: | |
| You can perform rewrites in middleware by passing a URL path parameter to `next()`, and it allows you to rewrite the current `Request`. | |
| However, unlike when using `context.rewrite()` directly, `next(rewritePayload)` will not execute a new rendering phase, and the middleware won't be called again, | |
| The following example shows rewriting the current `Request` to display the content from a login page when a user isn't logged in, instead of redirecting them to the login page itself: |
If I understand this correctly, next() will execute that function, but in the case of middleware, it's not something they write themselves here. And, given that you have a caution at the bottom of the section, I think it makes sense to spell out exactly what is happening here.
It's maybe more helpful to prepare the reader for what they'll see in the examples below, which is next() doing the actual rewriting under the hood?
There was a problem hiding this comment.
I removed this part:
This executes
context.rewrite()using the provided value (rewritePayload)
Because it's incorrect. They are two separate functions with two separate business logic.
|
I think we achieved what I wanted and what you wanted :D I am happy with the final changes |
Description (required)
This PR documents the rewrite APIs, which we plan to release in the next minor
For Astro version:
4.13. See astro PR #11542.