quality of life updates for App
#9579
Conversation
🦋 Changeset detectedLatest commit: abd6986 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
There was a problem hiding this comment.
Here's some preliminary feedback: since there isn't an issue nor an RFC where reviewers can understand the reasons and use cases of these additions, I suggest expanding the description of the PR, and telling us why these APIs were added, with possible use cases.
This will help us reviewers to have a better context and help you to land these new features.
|
Thanks, added a section to the description. |
Could you add a couple of examples? |
There was a problem hiding this comment.
I would like to see some tests to make sure that the new APIs behave as expected. We can add these tests in a separate PR if you want, but I think it's best to have them there, together with the new logic.
We have a test adapter, we can use that one to run some cases
|
Yeah, that makes sense. |
github-actions
bot
added
pkg: solid
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
github-actions
bot
removed
pkg: solid
|
@lilnasy I rebased this PR and the |
|
Thanks for the rebase! |
* quality of life updates for `App` (#9579) * feat(app): writeResponse for node-based adapters * add changeset * Apply suggestions from code review Co-authored-by: Emanuele Stoppa <my.burning@gmail.com> * Apply suggestions from code review Co-authored-by: Emanuele Stoppa <my.burning@gmail.com> * add examples for NodeApp static methods * unexpose createOutgoingHttpHeaders from public api * move headers test to core * clientAddress test * cookies test * destructure renderOptions right at the start --------- Co-authored-by: Emanuele Stoppa <my.burning@gmail.com> * Fallback node standalone to localhost (#9545) * Fallback node standalone to localhost * Update .changeset/tame-squids-film.md * quality of life updates for the node adapter (#9582) * descriptive names for files and functions * update tests * add changeset * appease linter * Apply suggestions from code review Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com> * `server-entrypoint.js` -> `server.js` * prevent crash on stream error (from PR 9533) * Apply suggestions from code review Co-authored-by: Luiz Ferraz <luiz@lferraz.com> * `127.0.0.1` -> `localhost` * add changeset for fryuni's fix * Apply suggestions from code review * Apply suggestions from code review Co-authored-by: Emanuele Stoppa <my.burning@gmail.com> --------- Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com> Co-authored-by: Luiz Ferraz <luiz@lferraz.com> Co-authored-by: Emanuele Stoppa <my.burning@gmail.com> * chore(vercel): delete request response conversion logic (#9583) * refactor * add changeset * bump peer dependencies * unexpose symbols (#9683) * Update .changeset/tame-squids-film.md Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca> --------- Co-authored-by: Arsh <69170106+lilnasy@users.noreply.github.com> Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com> Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com> Co-authored-by: Luiz Ferraz <luiz@lferraz.com> Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Why?
This came from working on 5 adapters over the past month. The logic for dealing with cookies and
node:httpis always duplicated.NodeApptakes care of converting the request, but the logic of converting response is duplicated across the node and the vercel adapters. Cookies had to be handled manually for legacy reasons (I think), but all adapters we maintain are now doing the same thing.Changes
App.Symbol.locals,App.Symbol.clientAddress)NodeApp.App.getSetCookieFromResponse()in favor ofapp.setCookieHeaders()(confusing name)app.render(request, { addCookieHeader: true })Testing
Tests for most of the functionality here are already present. This PR only exposes the APIs on App and NodeApp to prevent duplication across adapters.
Docs
Pending