-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add extensions to request payload & UI #3409
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 69c24ff The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@acao could you please grant me permission to run the workflow? I'd like to run through all the build checks |
@@ -28,6 +28,7 @@ export class HistoryStore { | |||
private shouldSaveQuery( | |||
query?: string, | |||
variables?: string, | |||
// extensions?: string, // dz todo: ask about saving strategy for extensions |
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.
In this method, there is logic to determine whether a query should be saved depending on the difference to the last saved query.
I would prefer to not add more logic for extensions here, but I wanted to highlight this in case you had something in mind.
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.
ah so - this will be important for extensions because it's how unique tabs are determined, and how extension input values can be saved in history and/or active tabs. you could leave this out for now, but then the lack of persistence will be reported as a bug knowing the expectations of users
--- | ||
'graphiql': minor | ||
'@graphiql/react': minor | ||
'@graphiql/toolkit': minor |
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 set these as minor changes but I am happy to be corrected. I chose minor as
- an empty Extensions tab will result in the same behaviour as before (no
extensions
key in the request payload) FetcherParams
are backwards compatible, it is fine to not setextensions
export type FetcherParams = {
query: string;
operationName?: string | null;
variables?: any;
extensions?: any;
};
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.
@dondonz agreed on the minor release, and users will appreciate the backwards compatibility!
@@ -44,7 +44,7 @@ this repo._ | |||
If you are focused on GraphiQL development, you can run — | |||
|
|||
```sh | |||
yarn start-graphiql | |||
yarn dev-graphiql |
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.
Updating the documentation
@@ -28,6 +28,7 @@ export type FetcherParams = { | |||
query: string; | |||
operationName?: string | null; | |||
variables?: any; | |||
extensions?: any; |
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 key line which adds extensions
to the payload. Everything else in this PR is for the Extensions UI tab
activeSecondaryEditor === 'variables' | ||
? 'Variables' | ||
: 'Headers' | ||
} |
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 replaced this ternary with a basic method to accommodate the extensions tab (the third tab). I tried to not be clever and go for a plain method rather than a .capitalize for the tab name
if (currentTab === 'headers') { | ||
return 'Headers'; | ||
} | ||
}; |
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 looks like a pretty basic method, I thought I'd make it as simple as possible and not be clever with a .capitalize
Previously this was a ternary in the component, which was fine when there were only 2 tabs.
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.
nice that works. and it's still in the component scope so we can readily translate it later
editorContext.initialHeaders && | ||
isHeadersEditorEnabled | ||
? 'headers' | ||
: '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.
I had to remove the ternary and use plain old if statements when introducing the extensions tab (the third tab)
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.
Well I guess I could have kept going with nested ternaries but nobody wants to see that 😄
} catch { | ||
/* Parsing JSON failed, skip prettification */ | ||
} | ||
} |
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.
Support for prettifying
useEditorContext({ | ||
nonNull: true, | ||
caller: HistoryItem, | ||
}); |
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.
Add extensions to history items so they can be retrieved later
@acao I'm ready for a review, please let me know what you think. Thanks! |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3409 +/- ##
==========================================
+ Coverage 55.75% 55.91% +0.16%
==========================================
Files 110 112 +2
Lines 5243 5340 +97
Branches 1426 1463 +37
==========================================
+ Hits 2923 2986 +63
- Misses 1897 1922 +25
- Partials 423 432 +9
|
Hello, checking in, is there anything else you need from me? |
Hi @acao could you please review this PR? |
hi @dondonz, yes I can take a look today! |
@@ -288,6 +312,7 @@ export function EditorContextProvider(props: EditorContextProviderProps) { | |||
useSynchronizeValue(queryEditor, props.query); | |||
useSynchronizeValue(responseEditor, props.response); | |||
useSynchronizeValue(variableEditor, props.variables); | |||
useSynchronizeValue(extensionEditor, props.extensions); |
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.
awesome stuff! i would prefer this to be possible via improvements to the plugin ecosystem, however extensions
are already spec and 100% be a built-in feature of the execution context, though it will be tricky because as you can see there are many touchpoints. eventually each of these tabs will be provided by plugins, so someone could replace with a yaml and/or json schema react hook form implementation or some such
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.
Here's an example to illustrate:
say you have 4 tabs, each with the exact same query/variables/headers combination, but each with unique extension values. when you reload the browser, you'll only have one tab, because without extensions, the persistence only finds one unique operation context
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.
awesome work! looking forward to getting this shipped!
the only thing I want to add, as you can see from the comment, is persistence and query param support to the renderExample.ts, if that's ok?
I would also like to add an extensions response example to our test schema, just so we can validate that extension payload responses are coming through, I think they are already yeah? at work we've been using apollo sandbox so i haven't tried graphiql with a schema with extensions in a while and I've forgotten haha. i would assume it works or there would be a bug haha |
92c2d63
to
69c24ff
Compare
Thanks for the review!! I was a bit busy with other things at work but I also want to get this merged in as soon as I can. I'll work on putting through your changes. |
@dondonz thank you! |
This PR adds
extensions
to the GraphQL request payload and adds a new Extensions tab to the UI (alongside the Variables and Headers tabs). This PR addresses #3372.For more background on
extensions
in the request payload, see the GraphQL over HTTP specification regarding Request Parameters. It is a map of values with no restrictions on content. It's a useful place to send metadata such as development flags.I tried to keep Extensions UI functionality as close as possible to the existing Variables and Headers behaviour. This PR also adds extensions to history and enables prettifying for extensions.
What happens to the request if the Extensions tab is empty? If the Extensions tab is empty, the corresponding GraphQL request payload will not contain the
extensions
key. Although the spec states:I prefer to be consistent with the prior behaviour of not setting an
extensions
key, rather than settingextensions
to null, in case a service somewhere has not covered the null case.