This repository has been archived by the owner on Oct 1, 2024. It is now read-only.
react-graphql improvements #726
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lemonmade
commented
May 30, 2019
| @@ -52,10 +53,11 @@ export default function useGraphQLDocument< | |||
| [document, loadDocument], | |||
| ); | |||
|
|
|||
| return [ | |||
| document, | |||
| useAsyncAsset( | |||
There was a problem hiding this comment.
I also moved this back into this hook, where it feels more appropriate than the useQuery hook
| useAsyncAsset(id); | ||
| if (typeof window === 'undefined' && skip) { | ||
| return createDefaultResult(client, variables); | ||
| } |
There was a problem hiding this comment.
This is the early bailout
| [queryObservable, client, variables], | ||
| () => createDefaultResult(client, variables, queryObservable), | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [queryObservable, client, serializedVariables], |
There was a problem hiding this comment.
This is the fix for the result changing when variables were deep equal
| afterEach(() => { | ||
| teardownAsyncReactTasks(); | ||
| }); | ||
|
|
| const wrapper = new CustomRoot(element, context, { | ||
| render: element => render(element, context, options), | ||
| resolveRoot: root => root.find(element.type), | ||
| }); |
There was a problem hiding this comment.
The basic gist of this fix is that instead of a Root around the result of render(element), the Root just gets the raw element under test, and we thread through the render to the component that eventually renders the component to the DOM, since it is in charge of handling updated props.
GoodForOneFare
approved these changes
May 31, 2019
5 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes a couple of small improvements to react-graphql:
skiptouseQueryon the server, it no longer attempts to fetch anything at all (before, it would try to resolve an async GraphQL document, mark it as used, and actually run the query)useQueryis referentially equal (it always returned the same data, but the object itself was changed between renders, which is a bummer because it's a common one to use as an input to otheruseMemos)In fixing (2), I also made an improvement to
react-testing. Previously, callingsetPropson a custom mounted element would set props on whatever the component was wrapped in, not the component itself. The changes toreact-testingmakes it so that only the element itself is updated with new props, which removes the need for hacks like https://github.com/Shopify/web/blob/master/tests/modern/AppContext.tsx#L63-L66.