-
Notifications
You must be signed in to change notification settings - Fork 1k
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: add GenerateGraphiQLHeaders as new param in createGraphQLHandler #5512
feat: add GenerateGraphiQLHeaders as new param in createGraphQLHandler #5512
Conversation
✅ Deploy Preview for redwoodjs-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
I also noticed this issue which could be related? |
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.
We'll pair soon to go over how we might mock the credentials per auth provider
01dd7d3
to
eded49e
Compare
Co-authored-by: Peter Colapietro <[email protected]>
@alicelovescake I tried to resolve conflicts and gets tests to pass. Can your review so we can merge? Note: I had to add a null check the in the shared code for dbAuth when checking cookies. |
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.
@dthyresson Good catch on the checking for null case when checking the session cookie. I just changed the cookie from graphiql header to return undefined
so we don't have to do the extra null check. Otherwise, I think we are good to merge
const cookieFromGraphiqlHeader =
process.env.NODE_ENV === 'development'
? JSON.parse(event.body ?? '{}').extensions.headers.cookie
: undefined
@dthyresson made some minor fixes to get the tests passing. I don't have write access so if you think it looks good, you can merge! |
Draft PR