graphiql
graphiql copied to clipboard
fix: fix fetchError persisting across different fetchers
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"
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: dylanowen / name: Dylan Owen (a524471dca7a46e6090bb0af637654ff122cbd92, 1d0d5d4fea3e7651447d8f6998bb9a8d8e84d0af, cafe2b25b88dd6b632af84bc19990879f4886f79)
🦋 Changeset detected
Latest commit: cafe2b25b88dd6b632af84bc19990879f4886f79
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
@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 Report
Merging #2653 (a524471) into main (2d91916) will increase coverage by
3.75%
. The diff coverage is23.84%
.
:exclamation: 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%) |
:arrow_up: |
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.
@thomasheyenbrock thanks for the quick review! I'm still working on the CLA but should have that ready soon.
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 😆
@acao Sorry for the CLA delay but it should be all set now
looks related to https://github.com/graphql/graphiql/issues/2771
@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.
@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: https://github.com/graphql/graphiql/blob/48e7710a80739931565e1d472ed38136cc73865f/packages/graphiql/src/components/DocExplorer.tsx#L123 but I didn't realize they were changed before my merge.
ahh...there it is. Thanks for the explanation, makes sense now.
I've got the fix in with #2778.
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.
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.
Ahhh nice, even better 👍