fix: fix fetchError persisting across different fetchers

[dylanowen]

If you construct a GraphiQL instance with a bad fetcher fetchError gets persisted in SchemaContextType forever. You can still fetch a new schema but the DocExplorer will always show "Error fetching schema"

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: dylanowen / name: Dylan Owen (a524471, 1d0d5d4, cafe2b2)

[changeset-bot]

🦋 Changeset detected

Latest commit: cafe2b2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
graphiql	Patch
@graphiql/react	Patch

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

[thomasheyenbrock]

Thanks so much @dylanowen for catching this! That's indeed a bug that got in with one of the refactorings we did in the past months (most likely my bad 😓 ). The fix itself is perfect, just have some tiny nits!

Can you also add a changeset here that describes the fix? (I just noticed that this is still missing in our CONTRIBUTING.md 🙈 ). Just run yarn changeset in the repository root and the CLI will walk you through how to create a changeset which is just a markdown file that you need to commit and push to the branch.

[thomasheyenbrock]

@dylanowen and you need to sign the CLA in order to become a contributor. Just click on the details link next to the failing GitHub check, that should walk you through everything.

[codecov]

Codecov Report

Merging #2653 (a524471) into main (2d91916) will increase coverage by 3.75%.
The diff coverage is 23.84%.

❗ Current head a524471 differs from pull request most recent head cafe2b2. Consider uploading reports for the commit cafe2b2 to get more accurate results

@@            Coverage Diff             @@
##             main    #2653      +/-   ##
==========================================
+ Coverage   65.70%   69.46%   +3.75%     
==========================================
  Files          85       71      -14     
  Lines        5106     4208     -898     
  Branches     1631     1414     -217     
==========================================
- Hits         3355     2923     -432     
+ Misses       1747     1280     -467     
- Partials        4        5       +1     

Impacted Files	Coverage Δ
packages/codemirror-graphql/src/lint.ts	100.00% <ø> (ø)
packages/codemirror-graphql/src/results/mode.ts	47.05% <ø> (ø)
packages/codemirror-graphql/src/utils/hintList.ts	95.65% <ø> (ø)
...ckages/codemirror-graphql/src/utils/mode-indent.ts	0.00% <0.00%> (ø)
packages/codemirror-graphql/src/variables/mode.ts	79.48% <ø> (ø)
packages/graphiql-react/src/editor/whitespace.ts	100.00% <ø> (ø)
packages/graphiql-react/src/utility/debounce.ts	0.00% <0.00%> (ø)
packages/graphiql-react/src/editor/tabs.ts	5.66% <5.66%> (ø)
packages/codemirror-graphql/src/variables/lint.ts	47.61% <66.66%> (+0.63%)	⬆️
packages/codemirror-graphql/src/hint.ts	94.73% <100.00%> (ø)
... and 100 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[dylanowen]

@thomasheyenbrock thanks for the quick review! I'm still working on the CLA but should have that ready soon.

[acao]

thanks for this! @dylanowen make sure to sign the CLA bot with the email you used to sign the commits, it's very specific about this but won't tell you this is why 😆

[dylanowen]

@acao Sorry for the CLA delay but it should be all set now

review addressed

[acao]

Looks good to merge!

[pitgrap]

looks related to #2771

[jonathanawesome]

@dylanowen I'm working on a small PR (#2778 ) to update some styling that's unrelated to this PR, but getting a failing test that is part of this PR.

 expect(
   container.querySelector('.doc-explorer-contents .error-container'),
 ).toBeTruthy();

👆 this is failing because those selectors don't exist. In fact, I can't find a record of them existing at the time this PR was created.

@acao @thomasheyenbrock Shouldn't this failing test have been caught by now? Looks like this PR didn't go through the standard set of checks.

[dylanowen]

@jonathanawesome I'm pretty sure this is caused by the long lag time between my PR being tested and me getting my CLA signed. When I opened this PR those selectors existed:

graphiql/packages/graphiql/src/components/DocExplorer.tsx

Line 123 in 48e7710

<div className="doc-explorer-contents">

but I didn't realize they were changed before my merge.

[jonathanawesome]

ahh...there it is. Thanks for the explanation, makes sense now.

I've got the fix in with #2778.

[dylanowen]

Thanks for the quick fix! I was wondering if it'd be better to not depend on the DocExplorer for the test so I was working on

    let foundError = null;
    function ErrorIcon() {
      foundError = useSchemaContext({nonNull: true}).fetchError;
      return <div/>;
    }
    const ERROR_PLUGIN = {
      title: 'test',
      icon: ErrorIcon,
      content: () => <div />,
    };
    function firstFetcher() {
      return Promise.reject('Schema Error');
    }
    function secondFetcher() {
      return Promise.resolve(simpleIntrospection);
    }

    // Use a bad fetcher for our initial render
    const { rerender } = render(<GraphiQL fetcher={firstFetcher} plugins={[ERROR_PLUGIN]} />);
    await wait();

    expect(foundError).toBeTruthy()

    // Re-render with valid fetcher
    rerender(<GraphiQL fetcher={secondFetcher} plugins={[ERROR_PLUGIN]} />);
    await wait();

    expect(foundError).not.toBeTruthy();

But I'll defer to whatever solution you think is best.

[jonathanawesome]

I think you've a good point...the Doc Explorer isn't in the DOM by default anymore (it must have been when you added your test), so I added a fireEvent to the test to ensure that it's there.

I also added an additional test closer to the component.

These are good, the-way-the-user-sees-it tests, so I think they're sufficient.

[dylanowen]

Ahhh nice, even better 👍