- Notifications
You must be signed in to change notification settings - Fork 3.2k
feat(storage-*): include modified headers into the response headers of files when using adapters #12096
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
Conversation
📦 esbuild Bundle Analysis for payloadThis analysis was generated by esbuild-bundle-analyzer. 🤖
Largest pathsThese visualization shows top 20 largest paths in the bundle.Meta file: packages/next/meta_index.json, Out file: esbuild/index.js
Meta file: packages/payload/meta_index.json, Out file: esbuild/index.js
Meta file: packages/payload/meta_shared.json, Out file: esbuild/exports/shared.js
Meta file: packages/richtext-lexical/meta_client.json, Out file: esbuild/exports/client_optimized/index.js
Meta file: packages/ui/meta_client.json, Out file: esbuild/exports/client_optimized/index.js
Meta file: packages/ui/meta_shared.json, Out file: esbuild/exports/shared_optimized/index.js
DetailsNext to the size is how much the size has increased or decreased compared with the base branch of this PR.
|
DanRibbens left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I likely wouldn't have named this modifyResponseHeaders but I see that this is an existing name in our upload config so we have to go with it.
Looks good!
| @paulpopus This PR has introduced an issue in repo. I specifically faced in azure storage static handler. Error log ( I added some console logs in statichandler) and getFile: What worked for me is this: It will be great if you can incorporate this. |
| Oh thank you for this @princebansal, can you give me the failing config so I can add a failing test with the fix as well? If you want the commit credits as well I'd be happy to review a PR on this |
| Gist of azure config in My collection: To reproduce, you can try to access any media(upload type) document from payload get api url by keeping read access as true. But I actually did not change anything the config to fix it. I just changed the way the headers are supposed to be merged. Let me know if this is what you were looking for. |
| Hey @princebansal I looked deeper into this and I'm not sure that your solution is safe enough but it's hard to tell as I couldn't reproduce the problem at all which means its likely a difference in the setup of the Azure blob storage, I have a basic one so let me know if there's something specific to yours so I can reproduce. Instead of your current patch can you try this fix and let me know if that solves it? let initHeaders: Headers = { ...(response.headers.toJson({ preserveCase: true }) as unknown as Headers), }On line 34 from the original change |
| Hey @princebansal or anyone else encountering this issue, we have a PR that may fix this but without any reproduction we can't properly review and release it. It would be extremely helpful if a reproduction configuration is provided or at the very least confirmation that this canary version fixes the problem |
This PR makes it so that
modifyResponseHeadersis supported in our adapters when set on the collection config. Previously it would be ignored.This means that users can now modify or append new headers to what's returned by each service.
Also adds support for
voidreturn on themodifyResponseHeadersfunction in the case where the user just wants to use existing headers and doesn't need more control.eg:
Manual testing checklist (no CI e2es setup for these envs yet):