Add GraphQL lint to editor component mount (Fixes #1507)

[danielfrankcom]

The Problem

The problem appears to be the fact that the GraphQL editor is only updated at initialization time, and when the text in it changes.

The render() function gathers some state that is used to construct a variableTypes object, which is then used in the component HTML to specify the linter options. This option is used by the GraphQL extension of CodeMirror to produce the warnings that are shown in the images within #1507.

This function is only called on component initialization, however the request that fetches the schema from the server does not return until later, so the editor cannot be properly rendered at construction. Though the request returns with the schema, the editor is not re-rendered until the user edits the query, hence the warnings as shown in #1507.

Solution Requirements

In order for the render() function to build the variableTypes object which is required for the linting to take place, there are a few requirements:

1. The GraphQL schema must exist, and be stored in this.state.schema. It is passed to this function which produces the final object needed for linting.

When the GraphQL editor component is created, the schema is automatically pulled from the server and stored in the this.state.schema variable for use. The _fetchAndSetSchema function is asynchronously called here, however the initial render of the component occurs before the schema request returns.

2. this._documentAST must contain a representation of the user's query. It is needed here.

this._documentAST is defined as null in the component constructor, and is not set properly until the query body is changed by the user.

Proposed Solution

The proposed solution is to ensure that the 2 requirements mentioned above are fulfilled, and then to trigger a manual re-render of the component.

We fulfill the second requirement at construction time by defining this._documentAST to the parsed value rather than null.

Since the first requirement (a valid schema in this.state.schema) must necessarily contact the server, we execute the re-render once the request has returned.

Once the 2 requirements are fulfilled, we call performLint() on the editor to update the warnings, and then call forceUpdate() to re-render the component.

constructor(props: Props) {
    ...
    this._documentAST = parse(body.query);
    ...
}

componentDidMount() {
    (async () => {
      await this._fetchAndSetSchema(this.props.request);
      await this._queryEditor.performLint();
      this.forceUpdate();
    })();
}

Shortcomings

Here is a list of things that are potentially non-ideal about this solution, and might need to be improved if deemed important:

Unclear on previous initial value for this._documentAST

I am still unclear on why this._documentAST was defined as null originally, since the query body is available for parsing at construction time. Though I tested this and everything seems to work, there may be a reason that I'm missing that invalidates this solution. If the initial value must truly be null, then it would always be possible to parse the body after the schema request returns.

this._documentAST surrounded by try/catch

The only other places setting this._documentAST was here, although I couldn't call this function directly because it had a built in guard against calling the function when nothing had changed. This code had a try/catch like this one:

try {
  this._documentAST = parse(query);
} catch (e) {
  this._documentAST = null;
}

although I am uneasy about this since there is no comment explaining why an exception might occur and what the ramifications are. I used the same code to define the variable in the constructor, but there is probably room for improvement if anyone knows what we are attempting to catch here.

Warning is visible until schema request returns

Before the request for the schema returns, the warning is still visible as the linter cannot validate the variables in the editor. We could potentially hide the warning on initialization, although the rules around this could get confusing if there is no URL present or if the request never returns or something like that.

Comments

This is clearly a non-obvious problem, and I haven't added any comments to describe why things are being done like this. It might be valuable to add comments in select places, although I'm not sure what would be most helpful and where.

[welcome]

💖 Thanks for opening this pull request! 💖

To help make this a smooth process, please be sure you have first read the
contributing guidelines.

[danielfrankcom]

I pushed an update that hopefully fixes an issue that flow was warning about. It's technically possible that _queryEditor is null when the schema request returns, so I added a guard against that. In the standard case this shouldn't happen since the editor is initialized as part of the first render.

[danielfrankcom]

I'm not really sure how to resolve this issue. The performLint() method is an extension method that's added to the CodeMirror class by an addon, but flow doesn't seem to recognize its existence. I've tried a couple of things but I'm out of my depth working with that tool so if someone else could point me in the right direction that'd be great. I don't think the tests are truly "failing", it seems like the static inspection can't spot the addon method.

[gschier]

Hey @danielfrankcom, thanks for digging into this one!

I played a bit with your solution locally and found that only setting this._documentAST in the constructor (line 93) was enough to make the errors disappear for me and it wasn't necessary to call performLint later on.

Do you have any insight on this? Is there something I'm missing?

Here's the query I'm testing with:

Note, that you still run into troubles before the schema is fetched. Perhaps we should disable linting altogether if the schema doesn't exist?

---

Hmm... this change actually does that nicely. Returning null for the variable type map is enough to disable variable linting when the schema doesn't yet exist.

[gschier]

I think I've got it. I'm going to merge this in and make some tweaks. Thanks so much for getting this started! 😄

[welcome]

Congrats on merging your first pull request! 🎉🎉🎉 You're helping make Insomnia awesome! 🙌

[danielfrankcom]

Hi @gschier, thanks so much for taking a look at this 😄

I think you are actually correct, the _documentAST change is the only one that is required.

I had initially thought that there was a race condition between the schema request returning and the 2nd set of renders, so I manually rendered to ensure that a schema returning after the 2nd render would still work. Upon looking at it again I see that the _fetchAndSetSchema method is actually the one triggering the render() call so there's no race condition there.

Glad you could figure out a cleaner fix!