Allow Astro.locals to be set by adapters
#7049
Conversation
🦋 Changeset detectedLatest commit: 5649918 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
wrapperup
changed the title
Astro.locals to be set by adapters
|
Just a comment from Docs to say that the update to the Node README, as well as the linked (closed) Docs PR are both great docs additions! While Middleware is still experimental, however, we would prefer to keep all documentation of anything middleware-related to the one Middleware page in docs. I'm not sure what the status of this PR is (I just know that Docs maintainers have been pinged for review), but if this is accepted it might be an idea to wait to release this when middleware is no longer experimental? Another option would be to accept the Node README changes with some kind of indication and link to experimental middleware, but then we'd also have to remember to remove it when middleware is considered stable. (Which is why we're trying to keep all documentation limited to the one page marked experimental in docs.) Anyway, just commenting that the documentation looks good, as does the closed docs PR which we are happy to reopen if this PR merges, so I'm hopeful it all makes its way into docs eventually! |
There was a problem hiding this comment.
APIContext being removed from renderPage may be a controversial change. As far as I can tell, the only thing it was passed in for was to access the locals object, but I'm not sure if there was any planned additions that will require the entire APIContext to be passed in.
That's precisely the case, the context is passed only to access locals and validate it because that's when locals is actually used in the rendering phase.
The changes look good to me, although I'd like to have more tests:
- a test case where an error is thrown if
localsis not serialisable - a test case where an error is thrown if
localsis not an object
A bonus, maybe that can be followed by another PR (not mandatory, just a nice to have), an example where we show our how users can use it. What do you think?
| ...options, | ||
| origin, | ||
| pathname, | ||
| url, | ||
| params, | ||
| props, | ||
| }; | ||
|
|
||
| // We define a custom property, so we can check the value passed to locals | ||
| Object.defineProperty(context, 'locals', { |
There was a problem hiding this comment.
I noticed this is the same code we use somewhere else. Maybe it's worth having a shared function and making it more "DRY". This would allow us to not have repeated logic to maintain.
|
We are currently planning on this for the 2.6 release, about 3 weeks from now. At that time middleware will go stable and the docs changes can be accepted (with some possible improvements) how they are written now. |
There was a problem hiding this comment.
Docs has had their say! Thanks for providing us great stuff to work with, @wrapperup! 🥳
| app.use(express.static('dist/client/')) | ||
| app.use((req, res, next) => { | ||
| const locals = { | ||
| foo: 'bar' |
There was a problem hiding this comment.
Would love if this didn't use foo: bar but instead something real? (In docs, we've been using title: "New title" to demonstrate updating a property.)
But, non-blocking!
|
@wrapperup I'm not able to push changes to your repository. This PR needs to be merged with main in order to be mergeable. We'd love to get this in for 2.6 if possible. |
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
|
@wrapperup Hey, we are in the process of merging things today. If you can resolve the conflicts we can get this in for 2.6. Thank you! |
|
It seems we haven't managed to merge this PR. @wrapperup are you still interested in working on this PR? If not, please let us know, we can takeover your work. |
|
Closing in favour of #7385 |
Changes
This is the spiritual successor to #3934
Astro Middleware RFC withastro/roadmap#531
Adapters can now pass in an object to be used in
Astro.locals. This is super useful for passing in data from a server into Astro, like user sessions and other per-request data derived on the server outside of Astro.localsobject into various render APIslocalsproperty, just like APIContext.renderPage, sincelocalscan be accessed from the RenderContextlocalscan be passed in the request viaSymbol.for('astro.locals')APIContext being removed from renderPage may be a controversial change. As far as I can tell, the only thing it was passed in for was to access the locals object, but I'm not sure if there was any planned additions that will require the entire APIContext to be passed in.
Testing
I wrote a few SSR tests for Astro core and the Node adapter.
Docs
withastro/docs#3224
Also updated the readme for the node integration with an example.