Improvements for a better embedding several instances of GraphiQL on the same page

[OlegIlyenko]

Recently I implemented a small app that builds on top of GraphiQL and provides additional functionality like tabs, headers, etc. http://toolbox.sangria-graphql.org/graphiql

Along the way I faced several issues when I tried to include several instances of GraphiQL on the same page. I also tried to use single instance and control it's state via props based on the selected tab, but it did not really worked very well at all. I found "fragile workarounds" for all of the issues, but I would like to list some of the issues with possible solutions in order to provide a better alternatives in future.

• top-level container uses an id of the element which is not appropriate if one wants to include several instances on the same page. This also makes styling a bit harder. I think class would be a better choice here.
• GraphiQL always includes an event listener for a key press which also sub-optimal. It would be nice to make it optional and configurable via props. I implemented a very fragile workaround to disable it and registered a single listener for a whole page. This also means that I had to use _runQueryAtCursor which I'm not suppose to use from the outside (I assume that underscore == private). It would be nice to "officially" expose this method as well.
• CodeMirror inside of the GraphiQL has issues when rendered in a hidden (display: none;) div. The gutter size gets messed up a bit. I had to use a trick where I render an empty div on a tab change (first time only) and then, on a next "frame", i render the actual thing. I'm not 100% sure what the actual problem is :)

A minor thing to consider

GraphiQL uses box-sizing: content-box;. As far as i saw, many people and css frameworks, like bootstrap and foundation, start to adopt box-sizing: border-box;. it's easy to fix with

.graphiql-container * {
  box-sizing: content-box;
}

I'm not very familiar with this field, so i'm not sure what would be a better choice, but still maybe it's a point worth considering

[asiandrummer]

Hi @OlegIlyenko! These issues all look legitimate to me - I'll see what I can do soon. Meanwhile, if you'd like to take a stab at it yourself I'm all for it!

[OlegIlyenko]

sure, I can try to put a PR together during the next days, not sure I can address all points though. I also have concerns about backwards compatibility. For example changing id to a class would be a breaking change. Do you think we can let it through?

[asiandrummer]

For example changing id to a class would be a breaking change. Do you think we can let it through?

I feel like we have a couple of choices here. One would be generating a breaking change and substitute id for class - in this case we will have to do a minor release instead of a patch for it. Another would be using them at the same time:

<div id="graphiql-container" class="graphiql-container">

BTW, although I agree that having id completely unique in the HTML document is a way to go, I'm a bit doubtful that having multiple instances of GraphiQL would be a general use case. Also, I'm curious of use cases that require the code to directly access the div#graphiql-container node. This train of thoughts convinces me that a thorough documentation of the breaking changes and a further support towards them would be a right choice here. What do you think?

[OlegIlyenko]

You brought up a good point. I also doubt that that there is many projects that are relying on a top-level div id. I think that good release notes and clear migration path would be sufficient in this particular case.

[OlegIlyenko]

I tried to find a solution for CodeMirror rendering issue. Turns out that one needs to call cm.refresh() after CodeMirror was made visible on the page (and initial render was in a hidden div). There is even an autorefresh addon precisely for this purpose.

I tried to use this addon and it works for the most part. Unfortunately, for some reason, it does not work for variable editor if editor does not have any variables defined yet (in this case variable editor is rendered in collapsed state). 😿

One can explicitly call cm.refresh() in some places, but I'm not sure what would be the best place to put it in for every editor. I also not 100% sure how refresh behaves in terms of performance and whether it's ok to put it in some performance-critical places, like CodeMirrorSizer. I tried to put it in there and it seems to do the trick.

[asiandrummer]

@OlegIlyenko about the collapsed variable editor, does it still have a weird gutter size when opening it up? On top of my head this sounds very much possible that it might be a CodeMirror bug - some workaround I could think of is to render an empty JSON string when the editor is empty ({} for instance), or directly calling cm.refresh() on the variable editor only like you said.

[OlegIlyenko]

yeah, variable editor will have an issue with the gutter size if I expand it after page was loaded, so autorefresh addon does not handle this for some reason. To be honest, I'm not very happy how autorefresh addon implemented anyways - it heavily relies on a timeouts which can cause unpredictable behavior in some cases. So I feel that it's better to avoid it.

Explicitly calling cm.refresh() at the right moment feels like a better solution. I guess we just need to identify places where we can call it.

[asiandrummer]

@OlegIlyenko - sorry about a delayed response. I began to think if what you're doing is already towards a right approach. I figure we have two cases here: display: none and programmatically resizing DOMs. I think that, if possible, the rendering of GraphiQL should not be done until the DOM becomes available, so rendering <GraphiQL /> conditionally in this context makes sense. Currently programmatically resizing DOMs isn't being handled, and it seems a bit tricky without one good cross-browser solution to listen to DOM resize events.

Another option we may consider is to call cm.refresh() function(s) manually when there's a need for it, but outside of <GraphiQL /> component. I thought about using refs to grab <GraphiQL /> component (and the editor components within it) and then using getCodeMirror().refresh() to get the job done.

In any case, I was thinking GraphiQL should do as minimal re-rendering as possible. For the case of resizing DOM sizes using JavaScript to affect CodeMirror editors' styles, we could listen to DOM resize events and call a throttled version of cm.refresh(), and fight through the cross-browser compatibilities 😄 What do you think about this?

[OlegIlyenko]

To be honest I'm not quite sure either. The last option sounds like a good one if it will work with DOM resize events (it's still a bit of a mystery to me how and when does codemirror calculates the sizes).

The second option sounds like a good compromise as well. Though in this case I would prefer a single GraphiQL.refresh() function that will refresh codemirror in all editors at once. This will help with better component encapsulation. This generally will not solve the problem (I think), but it will make a workaround a bit simpler.

Generally I would trust your judgment on this one :) I'm not quite sure what would be the best solution considering the browser compatibility and development/maintenance effort.

I also updated the last item in my list. I thought about it for a while, and I think it's not right to put it as a TODO item. I guess it's a valid point to consider, but it does not necessarily belongs to this issue or even should be implemented now or later.