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

feat(nuxt): useResponseHeader and useResponseHeaders #27131

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jelmerdemaat
Copy link

🔗 Linked issue

Resolves #27075

📚 Description

Adds 2 new composables:

  • useResponseHeader
  • useResponseHeaders

See referenced issue for details.

I tried my best to cover enough ground to make a draft PR:

  1. Proposed code for new composables
  2. Written 2 new documentation pages

But I still need some help :) With the following questions:

  • I'm not familiar with writing tests, is it expected that I add tests and if so, where do I start?
  • Right now I propose to return nothing from the functions, but we could also return true if successful and false if failed? Any ideas for better return values?
  • On naming the functions. The name "use" responseHeader kinda implies a return value, as it implies consuming something... Would it be better to name them setResponseHeader? That would be in line with the h3 library but, be out of character for a Nuxt composable.
  • Do I put the Nuxt versions above the functions myself, or are these added later?
  • Typings: right now I used string for both header name and value. But the h3 packages includes a type HTTPHeaderName which seems perfect for name. But the other composables about headers also didn't use this type, so I didn't use it. Should I use it? Is this something we could improve for all composables about headers?

Copy link

stackblitz bot commented May 9, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@jelmerdemaat jelmerdemaat changed the title feat(composables): useResponseHeader and useResponseHeaders feat(nuxt): useResponseHeader and useResponseHeaders May 9, 2024
@danielroe
Copy link
Member

I agree with you - I think that useResponseHeader doesn't make sense as a way of setting response headers. I think it should more likely return a computed value, which allows both getting and setting response headers.

For example:

const someHeader = useResponseHeader('x-some-header')
console.log(someHeader.value) // existing value
someHeader.value = 'new value'

@jelmerdemaat
Copy link
Author

Ah yes that would make more sense in the context of composables.

(I do feel like it would be (or look like?) a ref('') value, and a reactive value seems kind of... weird for a response header? I feel like a response header should have very strong reliability and not be subject to reactive changes/side effects. But I can't really explain why it feels like this to me.)

Would this code below be the implementation idea we're looking for?

export function useResponseHeader (header: string) {
  if (import.meta.client) { return undefined }

  const event = useRequestEvent();

  if (!event) return undefined;

  const headers = getResponseHeaders(event);

  return computed({
    // getter
    get() {
      return headers[header];
    },
    // setter
    set(newValue) {
      if (!newValue) {
        return removeResponseHeader(event, header);
      }

      return setResponseHeader(event, header, newValue);
    }
  });
}

@jelmerdemaat
Copy link
Author

@danielroe Any thoughts on my proposed solution before I start building it?

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

Successfully merging this pull request may close these issues.

Suggestion for new composables: useResponseHeader(s) similar to existing useRequestHeader composable
2 participants