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

Feature/issue 1048 handle merging additional Request / Response properties #1132

Conversation

thescientist13
Copy link
Member

@thescientist13 thescientist13 commented Aug 2, 2023

Related Issue

resolves #1048

Depends on #1131

Summary of Changes

  1. Honor and merge all constructor options for Response
  2. Honor body (.text(), .json() and formData()) and method on Request objects
  3. "Lock down" content type header responses in test cases (Found that internal Font Resource plugin was not returning the correct content type - worth making a new issue?)
  4. Added new test cases

TODO

  1. Merge all applicable Request properties- Search page with POST and FormData examples thescientist13/greenwood-htmx#3
    • body
    • headers
    • query params
    • Others? - see Thoughts section below
  2. Merge all applicable Response properties
    • status
    • statusText ?
  3. Apply to all instance in serve lifecycle
    • return all header, like custom x-secret header in the POST /api/submit example in develop
  4. executeRouteModule
  5. Apply to Adapter plugins
  6. Apply to SSR page bundled output? - https://github.com/ProjectEvergreen/greenwood/blob/v0.29.0-alpha.1/packages/cli/src/lifecycles/bundle.js#L224
  7. Add test cases
  8. POST w/ request.formData
    • and adapter plugins
  9. Clean up console logs
  10. Clean up TODO items
    • issues/1048
    • issues/1008
  11. Documentation updates?
  12. ~~multipart/form-data~~ support (nice to have)? - support additional Request.body formats #1147

Thoughts / Questions

  1. We don't get much from each ctx.request from Koa, so we can't really flesh out more than this from an initial request. Not sure how much of a Request options is ultimately worth merging on or we can even support? Maybe a good discussion topic? - Web Standards Web Server (native `Request` / `Response` support) #1146
    CTX.REQUEST {
      method: 'GET',
      url: '/api/missing',
      header: { host: '127.0.0.1:8080', connection: 'close' }
    }
  2. Could maybe add a tracking issue to validate all body options. Not sure how to best handle / detect them in Koa for serialization though? Options like: - support additional Request.body formats #1147
    • URLSearchParams
    • Blob

@thescientist13 thescientist13 added the feature New feature or request label Aug 2, 2023
@thescientist13 thescientist13 self-assigned this Aug 2, 2023
@thescientist13 thescientist13 changed the base branch from master to release/0.29.0 August 2, 2023 02:10
@thescientist13 thescientist13 changed the title Feature/issue 1048 handle merging all response properties Feature/issue 1048 handle merging all Request / Response properties Aug 10, 2023
@thescientist13 thescientist13 force-pushed the feature/issue-1048-handle-merging-all-response-properties branch from 0cce9f0 to 199c357 Compare August 16, 2023 01:08
@thescientist13 thescientist13 added alpha.3 v0.29.0 bug Something isn't working and removed alpha.3 v0.29.0 labels Aug 16, 2023
@thescientist13 thescientist13 changed the title Feature/issue 1048 handle merging all Request / Response properties Feature/issue 1048 handle merging additional Request / Response properties Aug 23, 2023
@thescientist13 thescientist13 marked this pull request as ready for review August 26, 2023 12:18
@thescientist13 thescientist13 force-pushed the feature/issue-1048-handle-merging-all-response-properties branch from 7a68bcc to 923a416 Compare August 26, 2023 16:54
@thescientist13 thescientist13 mentioned this pull request Aug 26, 2023
25 tasks
@thescientist13 thescientist13 merged commit 65a80d3 into release/0.29.0 Aug 26, 2023
8 checks passed
@thescientist13 thescientist13 deleted the feature/issue-1048-handle-merging-all-response-properties branch August 26, 2023 17:34
thescientist13 added a commit that referenced this pull request Nov 9, 2023
…roperties (#1132)

* support merging Response status property

* support body on incoming requests

* handle merging all custom response headers

* lock down content-type headers in test cases

* handle Response.statusText property

* full response support clean and TODOs cleanup

* update vercel adapter plugin specs for request and response handling

* update netlify adapter plugin specs for request and response handling

* add support for request.formData

* add request.formData support to adapter plugins

* variable name safe handler alias
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Adapter bug Something isn't working CLI feature New feature or request SSR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

handle merging additional Request / Response instance properties
1 participant