feat(astro): experimental middleware #6721
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: e8e5007 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
pkg: astro
This comment was marked as off-topic.
This comment was marked as off-topic.
|
This PR is blocked because it contains a |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
ematipico
force-pushed
the
feat/middleware-api
branch
from
e05bfe0 to
033173a
Compare
|
This PR is blocked because it contains a |
|
!preview middleware |
ematipico
force-pushed
the
feat/middleware-api
branch
from
033173a to
35c8652
Compare
|
This PR is blocked because it contains a |
ematipico
force-pushed
the
feat/middleware-api
branch
from
35c8652 to
ce9013d
Compare
|
This PR is blocked because it contains a |
|
ematipico
force-pushed
the
feat/middleware-api
branch
from
ce9013d to
6804f5c
Compare
|
This PR is blocked because it contains a |
ematipico
force-pushed
the
feat/middleware-api
branch
from
6804f5c to
c4acb57
Compare
|
This PR is blocked because it contains a |
|
!preview middleware |
ematipico
force-pushed
the
feat/middleware-api
branch
3 times, most recently
from
4e5a017 to
f844c7c
Compare
|
|
!preview middleware |
|
ematipico
force-pushed
the
feat/middleware-api
branch
2 times, most recently
from
fb79199 to
a724a92
Compare
matthewp
approved these changes
May 3, 2023
|
@sarah11918 could you double-check again if the docs are correct, please? |
bluwy
reviewed
May 3, 2023
sarah11918
reviewed
May 3, 2023
sarah11918
reviewed
May 3, 2023
Co-authored-by: Sarah Rainsberger <[email protected]>
bluwy
approved these changes
May 3, 2023
sarah11918
reviewed
May 3, 2023
Comment on lines
676
to
692
| * Thrown in development mode, when `locals` are overridden with something that is not an object | ||
| * | ||
| * For example: | ||
| * ```ts | ||
| * import {defineMiddleware} from "astro/middleware"; | ||
| * export const onRequest = defineMiddleware((context, next) => { | ||
| * context.locals = 1541; | ||
| * return next(); | ||
| * }); | ||
| * ``` | ||
| */ | ||
| LocalsNotAnObject: { | ||
| title: 'Value assigned to `locals` is not accepted.', | ||
| code: 3033, | ||
| message: `The \`locals\` can only be assigned to an object. Other values like numbers, strings, etc. are not accepted.`, | ||
| hint: 'If you tried to remove some information from the `locals` object, try to use `delete` or set the property to `undefined`.', | ||
| }, |
There was a problem hiding this comment.
Suggested change
| * Thrown in development mode, when `locals` are overridden with something that is not an object | |
| * | |
| * For example: | |
| * ```ts | |
| * import {defineMiddleware} from "astro/middleware"; | |
| * export const onRequest = defineMiddleware((context, next) => { | |
| * context.locals = 1541; | |
| * return next(); | |
| * }); | |
| * ``` | |
| */ | |
| LocalsNotAnObject: { | |
| title: 'Value assigned to `locals` is not accepted.', | |
| code: 3033, | |
| message: `The \`locals\` can only be assigned to an object. Other values like numbers, strings, etc. are not accepted.`, | |
| hint: 'If you tried to remove some information from the `locals` object, try to use `delete` or set the property to `undefined`.', | |
| }, | |
| * Thrown in development mode when `locals` is overwritten with something that is not an object | |
| * | |
| * For example: | |
| * ```ts | |
| * import {defineMiddleware} from "astro/middleware"; | |
| * export const onRequest = defineMiddleware((context, next) => { | |
| * context.locals = 1541; | |
| * return next(); | |
| * }); | |
| * ``` | |
| */ | |
| LocalsNotAnObject: { | |
| title: 'Value assigned to `locals` is not accepted', | |
| code: 3033, | |
| message: '`\locals\` can only be assigned to an object. Other values like numbers, strings, etc. are not accepted.', | |
| hint: 'If you tried to remove some information from the `locals` object, try to use `delete` or set the property to `undefined`.', | |
| }, |
Lots of things I'm unsure of! 😅 So these are just things I'll point out that might need a closer look to make sure it's what you mean/want:
- I might consider
localssingular, even though the word is plural? If it's onelocalsobject, then I think singular fits? So I'd say "locals is..." localscould be "overriden" or could be "overwritten" depending on what's actually happening. I just want to make sure that "override" is what you're going for, and you didn't actually prefer or mean "overWRITE". (I think of override in the sense of controls, or what's going to take precedence. But, I would probably think "overwrite" if the object contents are in fact being changed/rewritten.)- I'm not sure about the backtick symbols being used in the
messageabove. I changed the outer ones to single quotes, but I'm not sure exactly what you want within code ticks around locals. So just check on that and make it wahtever it should be. 😄 - the other
titles did not have periods at the end, so I removed this one, but whatever you want for titles is fine. Just pick a style!
There was a problem hiding this comment.
might consider locals singular, even though the word is plural? If it's one locals object, then I think singular fits? So I'd say "locals is..."
Yes, I would consider locals like a name, other than something we quantify.
locals could be "overriden" or could be "overwritten" depending on what's actually happening. I just want to make sure that "override" is what you're going for, and you didn't actually prefer or mean "overWRITE". (I think of override in the sense of controls, or what's going to take precedence. But, I would probably think "overwrite" if the object contents are in fact being changed/rewritten.)
I mean "override". The user can overwrite the contents of locals as they want. The user is free. But they can't do something like locals = "new string" (override). That is forbidden and will throw this very error.
Co-authored-by: Sarah Rainsberger <[email protected]>
sarah11918
reviewed
May 3, 2023
| title: '`Astro.locals` are not serializable.', | ||
| code: 3034, | ||
| message: (href: string) => { | ||
| return `The information stored in \`Astro.locals\` are not serializable when visiting "${href}" path.\nMake sure you store only data that are serializable.`; |
There was a problem hiding this comment.
Suggested change
| return `The information stored in \`Astro.locals\` are not serializable when visiting "${href}" path.\nMake sure you store only data that are serializable.`; | |
| return `The information stored in \`Astro.locals\` is not serializable when visiting "${href}" path.\nMake sure you store only serializable data.`; |
Information is singular, and I'm not sure about "when visiting... path" If that renders to e.g. "when visiting /about/ path" then that sentence wouldn't read very well in English.
What exactly is the signifiance of including the part about the path? Would the warning work without it, or is the message depending on knowing which path is causing the error? In that case, instead of "when visiting the path" what is the actual problem? The route can't be created? The path doesn't exist?
The information stored in Astro.locals for the path "${href} is not serializable. Make sure...
Would something closer to that be possible?
There was a problem hiding this comment.
What exactly is the signifiance of including the part about the path?
The logic of the middleware can change based on the page the user is visiting. So having a path can help the developer understand the issue.
The information stored in Astro.locals for the path "${href} is not serializable. Make sure...
Would something closer to that be possible?
That's perfect!
sarah11918
reviewed
May 3, 2023
Merged
|
@ematipico you have a typo in description also it looks like MiddlewareHandler requires generic type passed to it |
ArmandPhilippot
added a commit
to ArmandPhilippot/astro-docs
that referenced
this pull request
Jul 29, 2024
* See withastro/astro#4986 for `site`, `generator`, `url`, `clientAddress`, `props` and `redirect` * See withastro/astro#6721 for `locals` * See withastro/astro#9021 for `preferredLocale` and `preferredLocaleList` * See withastro/astro#9101 for `currentLocale`
ArmandPhilippot
added a commit
to ArmandPhilippot/astro-docs
that referenced
this pull request
Jul 29, 2024
* See withastro/astro#4986 for `site`, `generator`, `url`, `clientAddress`, `props` and `redirect` * See withastro/astro#6721 for `locals` * See withastro/astro#9021 for `preferredLocale` and `preferredLocaleList` * See withastro/astro#9101 for `currentLocale`
ematipico
added a commit
that referenced
this pull request
Feb 5, 2025
Co-authored-by: Sarah Rainsberger <[email protected]>
ematipico
added a commit
that referenced
this pull request
Feb 5, 2025
Co-authored-by: Sarah Rainsberger <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
"astro"package to use the preview release:src/, createmiddleware/index.jsor a file calledsrc/middleware.js. Or.ts; it's up to you.onRequestYou can manipulate the object to your heart content.
.astrofile (any file), access tolocalssequenceAPI provided by theastro/middlewaremodule:resolvefunction to retrieve and act on the response. When usingresolve, the middleware must return the response.contextobject (APIContexttype). It can becontext.request.url,context.params, etc.This PR implements the Astro middleware under an experimental flag. Please refer to the RFC for more information.
Testing
I created multiple test cases for
developmentandproductionmodes. The production tests use the preview feature and don't use an SSR adapter.Docs
cc
cc @withastro/maintainers-docs for feedback! Here's an issue where we can collect feedback: withastro/docs#3069