fix(GraphiQL) fallback to defaultQuery if there's no stored query

[rmosolgo]

Hi there, is GraphiQL correctly falling back to props.defaultQuery?

I find that if I open http://graphql-swapi.parseapp.com/ with an incognito window, the query window is blank. But, I think it should include defaultQuery defined at the bottom of GraphiQL.js.

I think the problem is that this._storageGet("query") returns null (https://developer.mozilla.org/en-US/docs/Web/API/Storage/getItem) if it is unset -- but the we treat that as useful value since it's !== undefined.

So, can we test it against null instead?

I tried to write tests for this but I couldn't quite get it: node.textContent didn't contain the query string and component.state had query: null, so I think it may not have been initialized. Should I add a test for this? Happy to do it, but I'll need a little guidance :)

[asiandrummer]

@rmosolgo good catch! So as you see we try to differentiate what undefined and null mean when props are given one of those values - I think you're right on that localStorage should not expect undefined type at all. Also verified from https://www.w3.org/TR/webstorage/#dom-storage-getitem.

For tests, if you do:

const graphiQL = renderIntoDocument(<GraphiQL />);
expect(graphiQL.state).to.equal(defaultQuery); // either export defaultQuery from `GraphiQL` or copy the variable here

like GraphiQL-test.js already does, you should be able to check graphiQL.state and confirm that it's the default query provided in the bottom of GraphiQL component. The tricky part is that mocha doesn't know what window.localStorage is and you'd have to mock the functionality; I've found a simple way to do so from StackOverflow link here. With mocked localStorage, and without your changes, the test should return "null" in string type I think - please be sure to check that :D

[rmosolgo]

Ohhh thanks for the tip about mocking localStorage! I had written some tests but I didn't understand why they were failing. Now I see that _storageGet returns undefined if there is no _storage.

I had to modify that mock one tiny bit: it returned undefined in the case of a missing key, but it should return null! JavaScript is awesome 😩

How does this look? Is it important for me to move that mock to another module, or is it ok at the top of that file?

[asiandrummer]

Cool - I think it's ok at the top of that file. It's a bit concerning because we're not cleaning up after Object.defineProperty, but I think we're fine for now :)

Thanks for the contribution @rmosolgo!