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

NEBULA-701 Add includeCookies field to apollo-server #6014

Merged
merged 6 commits into from Jan 25, 2022

Conversation

SilverMaiden
Copy link
Contributor

This PR is the final code change part of NEBULA-701 - updating apollo server to accept the includeCookies option from LandingPageConfig.

Studio-ui PR with more context

JIRA

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 19, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f97b6f2:

Sandbox Source
Apollo Server Typescript Configuration
Apollo Server Configuration

@@ -21,6 +21,7 @@ export interface ApolloServerPluginLandingPageDefaultBaseOptions {
document?: string;
variables?: Record<string, string>;
headers?: Record<string, string>;
includeCookies?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the doc comment above document to mention this (or better, split it up so each of the four options here have a doc comment since that's how doc comments work).

Ideally you should also add this to LandingPageConfig as well as to the two lines that look like

  const { version, __internal_apolloStudioEnv__, footer, ...rest } = options;

as explained in the comment above them. Those lines should probably also have document, variables, and headers added, oops... I guess we're kind of in the "haven't quite updated the plugin yet" state for them. The model here is a bit odd but basically works.

Please also update the doc website in docs/source/api/plugin/landing-pages.md. There's a README in docs explaining how to run the docs site locally; you can also see a Netlify deployment preview on the PR checks section.

Copy link
Member

@glasser glasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other note: we'll need to include this in CHANGELOG.md. Feel free to add a line yourself or I can do it when I merge it.

*/
document?: string;
/**
* Folks can configure their landing page to link to Studio Explorer with
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that it's written four times I'm starting to wonder if "folks can" is the best way to start a doc string. I kinda love it and I kinda hate it. What do you think?


A boolean used to set whether cookies are included in the Studio Explorer's editor on load.

If you omit this, the Explorer defaults includeCookies to 'false' or the current user setting.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

includeCookies should probably be in backticks here. probably false too.

</td>
<td>

A boolean used to set whether cookies are included in the Studio Explorer's editor on load.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could a bit more explicit, like, "A boolean used to set whether Studio Explorer should include cookies in its GraphQL requests to your server." ?

@glasser
Copy link
Member

glasser commented Jan 25, 2022

Looks good to me @SilverMaiden ! I'll add a CHANGELOG entry and merge it.

@uripre
Copy link

uripre commented Feb 25, 2022

That's awesome, thanks!!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants