-
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-1385: Add initialState to embedable sandbox based on config #6628
NEBULA-1385: Add initialState to embedable sandbox based on config #6628
Conversation
✅ Deploy Preview for apollo-server-docs canceled.Built without sensitive environment variables
|
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 30ed632:
|
|
@@ -142,6 +142,7 @@ id="embeddableSandbox" | |||
target: '#embeddableSandbox', | |||
initialEndpoint, | |||
includeCookies: ${config.includeCookies ?? 'false'}, | |||
initialState: ${config}, |
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.
initialState
should only have 3 properties, maybe better to be explicit about passing only what it expects?
initialState: ${config}, | |
initialState: ${{ | |
document: config.document ?? undefined, | |
variables: config.variables ?? undefined, | |
headers: config.headers ?? undefined, | |
}}, |
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.
@William010x and I identified that you can't just drop an object into a string template like this (you get an [object Object]
), so it's now JSON.stringify()
ed.
Additionally, I'll mention that the type of initialState
is now a string
and it'll need to be JSON.parse()
d in the constructor to be useful.
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.
With the substitutions in the template string (${JSON.stringify(expression)}
), it actually removes the quotes surrounding the JSON object string, so the returned HTML string will look something like:
<script>
var initialEndpoint = window.location.href;
new window.EmbeddedSandbox({
target: '#embeddableSandbox',
initialEndpoint,
includeCookies: false,
initialState: {"document":"query test { id }","variables":{"v":"1"},"headers":{"h":"2"}},
});
</script>
I tested this HTML string with the Embedded Sandbox and it seems to work without needing to JSON.parse()
.
Alternatively, doing something like these (similar to the embeddedExplorerHTML) also works
<script>
var initialEndpoint = window.location.href;
var state = ${JSON.stringify({
document: config.document ?? undefined,
variables: config.variables ?? undefined,
headers: config.headers ?? undefined,
})};
new window.EmbeddedSandbox({
target: '#embeddableSandbox',
initialEndpoint,
includeCookies: ${config.includeCookies ?? 'false'},
initialState: state,
});
</script>
<script>
var initialEndpoint = window.location.href;
var state = ${JSON.stringify({
document: config.document ?? undefined,
variables: config.variables ?? undefined,
headers: config.headers ?? undefined,
})};
new window.EmbeddedSandbox({
target: '#embeddableSandbox',
initialEndpoint,
includeCookies: ${config.includeCookies ?? 'false'},
initialState: {
...state
},
});
</script>
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'll look into adding proper unit tests for this though!
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.
Neat, yeah if it's not adding quotes like I expected then you should be all set!
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.
This LGTM.
A couple last notes:
- The
variables
type as it's written in the docs change doesn't allow for a nested object. I wish we had a good "one-liner" type to put there, but we really don't. The recursive nature of this type requires writing one type in terms of itself. Options which I'd consider an improvement but still not ideal:Record<string, string | Record>
,Record<string, any>
. - We sadly have no unit tests for the html rendered by the plugin. It would be nice to see new tests that reflect this change. I won't block you on this, but I'd be happy to work with you to get a few in place. Otherwise, opening a follow-up issue would be the next best option so it's at least not forgotten.
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.
wow thank you for the tests 🙏
@@ -106,6 +106,52 @@ By default, the landing page displays a footer that links to the documentation t | |||
<tr> | |||
<td> | |||
|
|||
###### `document` |
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.
do these get passed through to sandbox in the regular landing page? We were just missing them before but they are supported right?
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.
yup! they were always in ApolloServerPluginLandingPageDefaultBaseOptions
, but just not mentioned in the docs
</td> | ||
<td> | ||
|
||
A GraphQL document (eg, query or mutation) to populate in the Studio Explorer's editor on load. |
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.
A GraphQL document (eg, query or mutation) to populate in the Studio Explorer's editor on load. | |
A GraphQL document (eg, query or mutation) to populate in the Studio Sandbox Explorer's editor on load. |
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.
Do we still want to add this? This API applies to both sandbox and explorer, right?
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.
This is the local dev docs, so they will only ever lead to sandbox afaict
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.
no strong opinion tho!
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.
Thanks for adding these tests! I'm a lot more confident in the HTML we're expecting now 👍
A few minor comments mostly about structure, otherwise this is great.
Would you please add an entry to the vNEXT
section of CHANGELOG.md
? A small description with a link to your PR is great, you can look to the others for example.
|
||
describe('Landing Page Config HTML', () => { | ||
it('for embedded explorer with document, variables, headers and displayOptions provided', () => { | ||
const config = { |
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.
If you define config
as the actual options type (ApolloServerPluginEmbeddedLandingPageProductionDefaultOptions
) you won't need the type casts below. Right now TS infers the type of the object (with docsPanelState
and theme
as string
, which is not narrow enough). But if you tell it the object's type, it'll correctly give you an error if you used an incorrect string for those properties.
packages/apollo-server-core/src/__tests__/landingPageConfigHTML.test.ts
Outdated
Show resolved
Hide resolved
packages/apollo-server-core/src/__tests__/landingPageConfigHTML.test.ts
Outdated
Show resolved
Hide resolved
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.
LGTM! We'll want to port these changes over to the version-4
branch as well (ignoring any docs changes). If you have time for that, that would be great. If not, you can open an issue for it and assign to me.
Co-authored-by: Trevor Scheer <trevor.scheer@gmail.com>
@glasser can you review in post? I was thinking I'd release this, but since you're back tomorrow I'll hold off in case you want to recommend any follow-ups. |
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.
so just to check — this was basically a bug, where the TS types had these parameters but in one particular case (embedded) we weren't passing them through?
@@ -228,7 +274,7 @@ If you omit this, the Explorer initially loads an example query based on your sc | |||
|
|||
###### `variables` | |||
|
|||
`Record<string, string>` | |||
`Record<string, string | Record>` |
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.
maybe the right hand side here should just be unknown
or any
? i mean you can put numbers and bools and lists in variables...
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.
This ticket was for a new config (initialState) recently added to the embedded-explorer repo. These parameters (document, variables and headers) already existed, but yeah they weren't used or passed through for embedded Sandbox.
The parameters were added and the docs were updated accordingly
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.
Added this change in #6656
Context
The landing options/initial state (
document
,variables
,headers
) is not currently implemented in the embedded sandbox. This is being added in a studio-ui PR and embeddable-explorer PRJIRA.
What changed
document
,variables
,headers
that the user provides in ApolloServerPluginLandingPage to the embeddable sandbox