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

feat: Add Headers Editor to GraphiQL #1543

Merged
merged 29 commits into from
May 28, 2020
Merged

feat: Add Headers Editor to GraphiQL #1543

merged 29 commits into from
May 28, 2020

Conversation

connorshea
Copy link
Contributor

@connorshea connorshea commented May 22, 2020

This adds a header editor, for submitting HTTP headers when executing a query, as described in #59.

image

This is especially useful when you want to send an authentication token with your request, for example.

This adds an optional headers parameter to the fetcher which can then be merged into any other headers that the application sets itself.

import React from 'react';
import ReactDOM from 'react-dom';
import GraphiQL from 'graphiql';
import fetch from 'isomorphic-fetch';

function graphQLFetcher(graphQLParams, headers) {
  return fetch(window.location.origin + '/graphql', {
    method: 'post',
    headers: Object.assign(headers, { 'Content-Type': 'application/json' }),
    body: JSON.stringify(graphQLParams),
  }).then(response => response.json());
}

ReactDOM.render(<GraphiQL fetcher={graphQLFetcher} />, document.body);

TODO:

  • Add tests.
  • Make sure this isn't a breaking change, or if it is we need to warn users and make sure upgrading is as easy as possible.
  • Add the ability to disable the headers editor entirely.
  • Make the tab selection style less ugly.
  • Rewrite my commit messages to pass the linter.
  • Figure out how to get the development application to handle the headers as headers (probably by modifying the fetcher it's rendered with).
  • Probably add some way to set a default value for the headers editor?

@connorshea
Copy link
Contributor Author

connorshea commented May 22, 2020

I'll also need to create a compatibility layer for the defaultVariableEditorOpen prop (is there an example I can look at of how things should be deprecated in the library?), since I've renamed it (there are multiple tabs now, so the name defaultVariableEditorOpen doesn't make sense).

I don't really like the new name defaultExtraEditorOpen, so I'm open to suggestions if anyone has anything. Maybe defaultSecondaryEditorOpen?

@acao
Copy link
Member

acao commented May 22, 2020

@connorshea this is looking great!

yes that sounds good! for deprecating a prop, we can just note it in the readme.

@acao
Copy link
Member

acao commented May 22, 2020

fyi your code already passes the linter and prettier check, only thing failing now is unit/cypress tests

@acao acao added this to the GraphiQL-1.0.0 milestone May 22, 2020
@harshithpabbati
Copy link
Contributor

@connorshea great work!

I found two issues in this
video

In the above GIF you can see that when I click on the tab the editor is not being opened.

@harshithpabbati
Copy link
Contributor

And also, when I change the tab to query variables then the value is not visible when we come back to request headers.

video1

This way the editors are persisted when switching tabs.
…e-by-side.

Despite the other editor being rendered as display: none, the gutter for
some reason would get squished by CodeMirror. This is the only thing
I've figured out that will fix it.

I _think_ this works because setting position: absolute will take it
out of the normal document flow, but I'm not entirely sure.
@connorshea
Copy link
Contributor Author

@harshithpabbati I think I've fixed it so the editor contents persists now. It only came at the expense of a terrible hack to get the editor gutters to render properly when the page thought there were two editors rendered :)

The prop will default to true. Also fixed headers not being initialized
as part of the initial component state, this way default headers can be
passed into the component.
@connorshea
Copy link
Contributor Author

connorshea commented May 23, 2020

Should I add functions for toggleVariableEditor and toggleHeaderEditor that can be overridden by the user? Docs and History already have them, so I guess it'd make sense?:

handleToggleDocs = () => {
  if (typeof this.props.onToggleDocs === 'function') {
    this.props.onToggleDocs(!this.state.docExplorerOpen);
  }
  this.setState({ docExplorerOpen: !this.state.docExplorerOpen });
};

handleToggleHistory = () => {
  if (typeof this.props.onToggleHistory === 'function') {
    this.props.onToggleHistory(!this.state.historyPaneOpen);
  }
  this.setState({ historyPaneOpen: !this.state.historyPaneOpen });
};

EDIT: Actually, I guess it wouldn't be toggle since that doesn't make sense in this case. It'd be openHeaderEditor and openVariableEditor.

@connorshea
Copy link
Contributor Author

Unrelated to this PR, but I just noticed onToggleHistory isn't documented in the README.

@acao
Copy link
Member

acao commented May 25, 2020

@harshithpabbati maybe someone could do a layout like that in 2.0.0 but this design is more consistent

@acao
Copy link
Member

acao commented May 25, 2020

@connorshea another tweak would be to use grayscale instead of red for the accent/activity color, otherwise it's looking pretty good!

@acao
Copy link
Member

acao commented May 26, 2020

@connorshea are you able to make that final change?

@acao
Copy link
Member

acao commented May 27, 2020

I will just go ahead and merge, and adjust the colors in another PR. thanks for all this work!

@connorshea
Copy link
Contributor Author

@acao the active/inactive tabs are now black and gray and I've added the FetcherOpts change.

@acao
Copy link
Member

acao commented May 27, 2020

@connorshea thank you! sorry if it's kinda boring, just wanted to make sure it's neutral for now. the more exciting design is for 2.0.0!

@acao
Copy link
Member

acao commented May 27, 2020

ah hm, something about the cypress suite executing queries, and JSON parsing, interesting. you should be able to run yarn e2e from the root if you like. let me know if you have any issues

@connorshea
Copy link
Contributor Author

@acao no problem, I was fully aware it didn't look very good in the initial implementation :)

I'll take a look at the cypress tests.

@connorshea
Copy link
Contributor Author

@acao should be good to go now :)

@acao
Copy link
Member

acao commented May 28, 2020

@connorshea oops! so my suggestion to change the text color actually introduced an accessibility debt, my bad.

I'm going to make another PR to address a few other issues I discovered in a cursory, automated runtime audit

@acao acao changed the title Add Headers Editor to GraphiQL feat: Add Headers Editor to GraphiQL May 28, 2020
@acao acao merged commit 3faa1ac into graphql:master May 28, 2020
@connorshea connorshea deleted the add-headers-editor branch June 1, 2020 20:17
@justinmchase
Copy link

Which version of graphql would this have landed in?

I just went and got the latest from npm "graphql": "^15.5.0" and have set graphiql: true but this feature appears to be missing.

@harshithpabbati
Copy link
Contributor

Hi, you need to pass in headerEditorEnabled prop, to get the header editor tab. It's default to false.

You can look at the props here.

@justinmchase
Copy link

@harshithpabbati

Awesome, I see it now. I changed it to graphiql: { headerEditorEnabled: true } and that did it. Probably seems like a setting that should be true by default but thanks for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants