-
Notifications
You must be signed in to change notification settings - Fork 2k
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-2126: Add new config options to Sandbox & Explorer landing pages #7431
NEBULA-2126: Add new config options to Sandbox & Explorer landing pages #7431
Conversation
👷 Deploy request for apollo-server-docs pending review.Visit the deploys page to approve it
|
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. |
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.
Changeset please (and I'm ignoring changes from the other PR)
collectionId?: string; | ||
operationId?: string; |
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 don't expect one without the other here, right? I think I'd suggest something like:
defaultOperation?: {
collectionId: string;
operationId: string;
}
That way we can give them a TS error for one without the other. (or you could get funky with the typings to keep them top level, but I think I like the nesting and the naming)
Another idea that comes to mind that we probably don't want but hey, just in case. A single string "ref" like: collectionId@operationId
which wouldn't feel too unfamiliar because graph refs.
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 wrapped! Just noting that I added this to the shared types, but its only supported in the embeds, I just moved it out of this shared base type into the specific embed: true types 👍🏻
* | ||
* The default value is true. | ||
*/ | ||
pollForSchemaUpdates?: boolean; |
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.
First thing I do when I load up a sandbox. Glad we're adding this!
packages/server/src/plugin/landingPage/default/getEmbeddedHTML.ts
Outdated
Show resolved
Hide resolved
packages/server/src/plugin/landingPage/default/getEmbeddedHTML.ts
Outdated
Show resolved
Hide resolved
@trevor-scheer flagging that I am fixing this in this PR too https://github.com/apollographql/apollo-server/pull/7358/files#r1130206916 |
@mayakoneval just bureacracy left:
|
7a31290
to
73f2218
Compare
This PR has commits from the bug fix PR and This PR which is rebased off of the bug fix PR added the snippet to this changeset as well 🙏🏻 |
2818ed5
to
8bdd98d
Compare
TODO for maya in the morning, test several config options & make sure embedded explorer shows up as expected w the operation, collection, shared headers, uneditable url box etc |
@mayakoneval q: what's the expected behavior for providing both this new config and a Optionally, document could become |
/** | ||
* Headers that are applied by default to every operation executed by the landing page's Sandbox. | ||
* Users can disable the application of these headers, but they can't modify their values. | ||
* | ||
* The landing page's Sandbox always includes these headers in its introspection queries | ||
* to your endpoint. | ||
*/ | ||
sharedHeaders?: Record<string, string>; | ||
}; |
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.
@cheapsteak @trevor-scheer noting a weirdness here.
This was added in response to this issue, where users expect headers
to be applied to their introspection request. This would be a major update b/c the behavior would change with no config change. I added sharedHeaders
as an option instead, but it will be confusing for folks to have a top level headers
option and then a sharedHeaders
option nested in embed: { initialState: { sharedHeaders: {} } }
. I don't see a way around this but will add a TODO
for the next major version perhaps? What do you think?
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.
What's the desired outcome for the API? A TODO(as5)
, // @deprecated
, and an actual console.warn
deprecation warning when we detect bad usage with helpful messaging would be good if people can migrate to a correct thing now.
If they can't migrate to the thing we want it to be yet, a TODO(as5)
w/comment is sufficient.
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.
They can't migrate now, headers
at the base level will populate headers into the default tab on their Explorer, while sharedHeaders
will populate the shared headers in Sandbox that are sent to introspection. These are semantically different, but in AS5 we should probably switch their behavior. 🙏🏻 added TODO!
an operation from a collection includes headers, variables and a document actually. Now I am thinking I should have changed the types of the embed to be
The issue with the landing pages is that we don't support |
8bdd98d
to
3a169a3
Compare
… includes the introduction of config options for the Sandbox aka the local dev embedded landing page option. New config options for Sandbox are: pollForSchemaUpdates, endpointIsEditable, collection & operation id embedding. For explorer, it is just collection & operation id embedding
…nId & operationId in a defaultOperation obj
…out of shared types
…cument, headers, variables
3a169a3
to
f3134e0
Compare
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/server@4.5.0 ### Minor Changes - [#7431](#7431) [`7cc163ac8`](7cc163a) Thanks [@mayakoneval](https://github.com/mayakoneval)! - In the Apollo Server Landing Page Local config, you can now automatically turn off autopolling on your endpoints as well as pass headers used to introspect your schema, embed an operation from a collection, and configure whether the endpoint input box is editable. In the Apollo Server Landing Page Prod config, you can embed an operation from a collection & we fixed a bug introduced in release 4.4.0 Example of all new config options: ``` const server = new ApolloServer({ typeDefs, resolvers, plugins: [ process.env.NODE_ENV === 'production' ? ApolloServerPluginLandingPageProductionDefault({ graphRef: 'my-graph-id@my-graph-variant', collectionId: 'abcdef', operationId: '12345' embed: true, footer: false, }) : ApolloServerPluginLandingPageLocalDefault({ collectionId: 'abcdef', operationId: '12345' embed: { initialState: { pollForSchemaUpdates: false, sharedHeaders: { "HeaderNeededForIntrospection": "ValueForIntrospection" }, }, endpointIsEditable: true, }, footer: false, }), ], }); ``` - [#7430](#7430) [`b694bb1dd`](b694bb1) Thanks [@mayakoneval](https://github.com/mayakoneval)! - We now send your @apollo/server version to the embedded Explorer & Sandbox used in the landing pages for analytics. ### Patch Changes - [#7432](#7432) [`8cbc61406`](8cbc614) Thanks [@mayakoneval](https://github.com/mayakoneval)! - Bug fix: TL;DR revert a previous change that stops passing includeCookies from the prod landing page config. Who was affected? Any Apollo Server instance that passes a `graphRef` to a production landing page with a non-default `includeCookies` value that does not match the `Include cookies` setting on your registered variant on studio.apollographql.com. How were they affected? From release 4.4.0 to this patch release, folks affected would have seen their Explorer requests being sent with cookies included only if they had set `Include cookies` on their variant. Cookies would not have been included by default. ## @apollo/server-integration-testsuite@4.5.0 ### Patch Changes - Updated dependencies \[[`7cc163ac8`](7cc163a), [`8cbc61406`](8cbc614), [`b694bb1dd`](b694bb1)]: - @apollo/server@4.5.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This should be rebased off of this PR, can you do that with forks?
Context
JIRA
What Changed?
We recently added the ability to
in the embeddable-explorer repo
So this PR allows users to set the above configuration in their Sandbox. This PR also allows folks to pass operation ids to the Explorer, but no other changes are made the the Explorer.
How to test
erm, run the landing pages locally!
there are already tests for the html functions