Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle merging additional Request / Response instance properties #1048

Closed
Tracked by #1120
thescientist13 opened this issue Feb 4, 2023 · 1 comment · Fixed by #1132, #1120, #1148 or #1149
Closed
Tracked by #1120

handle merging additional Request / Response instance properties #1048

thescientist13 opened this issue Feb 4, 2023 · 1 comment · Fixed by #1132, #1120, #1148 or #1149
Assignees
Milestone

Comments

@thescientist13
Copy link
Member

thescientist13 commented Feb 4, 2023

Summary

As part of #948, we created a utility in resource-utils.js to merge Response objects when needed, but it doesn't currently support anything other body and headers.

return new Response(source.body, {
  headers
});

In addition, instances of Request are missing critical properties like body, which means Greenwood is totally unusable for handling POST requests
https://github.com/ProjectEvergreen/greenwood/blob/v0.28.5/packages/cli/src/lifecycles/serve.js#L37

  const initRequest = new Request(url, {
    method: ctx.request.method,
    headers: new Headers(ctx.request.header)
  });

Details

I would like to make sure that we can merge all relevant Response instance properties like

  • status
  • statusText
  • Custom headers like Content-Length

And Request instance properties

  • method
  • body (make sure it supports json, formData, etc)
  • integrity
  • referrer
  • referrerPolicy
  • all of them?

Needs to be done in


Bonus points for handling the Range header (good link here on implementation) which I think will fix this issue with serving media on Safari

Error: read ECONNRESET
at TCP.onStreamRead (node:internal/stream_base_commons:217:20)

Screen Shot 2023-03-03 at 9 09 28 PM

@thescientist13 thescientist13 added the feature New feature or request label Feb 4, 2023
@thescientist13 thescientist13 added this to the 1.0 milestone Feb 4, 2023
@thescientist13 thescientist13 changed the title handle merging all Response instance properties handle merging all Response instance properties and headers Feb 18, 2023
@thescientist13 thescientist13 self-assigned this Aug 2, 2023
@thescientist13 thescientist13 mentioned this issue Aug 2, 2023
25 tasks
@thescientist13 thescientist13 changed the title handle merging all Response instance properties and headers handle merging all Request / Response instance properties and headers Aug 10, 2023
@thescientist13 thescientist13 added P0 Critical issue that should get addressed ASAP Adapter CLI SSR alpha.3 and removed P0 Critical issue that should get addressed ASAP labels Aug 10, 2023
@thescientist13 thescientist13 changed the title handle merging all Request / Response instance properties and headers handle merging all Request / Response instance properties Aug 16, 2023
@thescientist13 thescientist13 changed the title handle merging all Request / Response instance properties handle merging additional Request / Response instance properties Aug 24, 2023
@thescientist13 thescientist13 moved this from 🏗 In progress to 👀 In review in [Greenwood] Phase 9 - Standards and Conventions Aug 26, 2023
@thescientist13 thescientist13 linked a pull request Aug 26, 2023 that will close this issue
25 tasks
@thescientist13
Copy link
Member Author

See #1146 and #1147 for follows up on this work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment