Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

5. Proxy GraphQL request from React to Shopify #14

Merged
merged 1 commit into from
May 11, 2021

Conversation

MuhammadFarag
Copy link
Contributor

@MuhammadFarag MuhammadFarag commented May 7, 2021

app-bridge-react and react-apollo

The plan was initially to mimic the implementation in https://github.com/Shopify/shopify-app-node/blob/master/pages/_app.js. However, app-bridge-react and react-apollo are not compatible with React 17, which is what Laravel is using. As a result I have opted to using Apollo client directly.

Potential issue with graphql proxy

Should be fixed when Shopify/shopify-api-php#74 is merged

Query variables get transformed from a JSON object to an array.
  • Requests coming from Apollo to the App come as a string for example: {"variables":{},"query":"{\n shop {\n name\n __typename\n }\n}\n"}
  • In Utils::graphqlProxy that payload is decoded with the flag for associative array result json_decode($rawBody, true)
  • When we try to re-encode the body again inside GraphQ->query it gets encoded as {"variables":[],"query":"{\n shop {\n name\n __typename\n }\n}\n"}

Todo:

  • Tests
  • In the Shopify App Library debug issue with graphQlProxy
  • Add X-Shopify-API-Request-Failure-Reauthorize-Url

@@ -11,7 +11,7 @@ class VerifyCsrfToken extends Middleware
*
* @var array
*/
protected $except = [
protected $except = ['graphql'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requests do not have csrf token, however I believe that verifying jwt is enough to ensure that the request is legit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

@MuhammadFarag MuhammadFarag changed the title Proxy GraphQL request from React to Shopify 5. Proxy GraphQL request from React to Shopify May 7, 2021
@MuhammadFarag MuhammadFarag marked this pull request as ready for review May 11, 2021 06:41
@MuhammadFarag MuhammadFarag requested a review from a team as a code owner May 11, 2021 06:41
Copy link
Collaborator

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe react-apollo was deprecated, so not using it is definitely the right call!

Looking great!

@@ -11,7 +11,7 @@ class VerifyCsrfToken extends Middleware
*
* @var array
*/
protected $except = [
protected $except = ['graphql'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

query: TEST_QUERY
})
.then(result => console.log(result));

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still use the other components of the node app here, but continue to use the apollo client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 59 to 48
$client = $this->createMock(ClientInterface::class);
$factory = $this->createMock(HttpClientFactory::class);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there's a simpler way of doing this - I feel like this test is relying a little bit too much in the library's implementation, and I'm wondering if it would be possible to mock the behaviour a little farther up the call tree so we mock library components / methods rather than its internal calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it depends on the library implementation. I am not sure if we can mock something up the stack though. We can do that exercise separately for the testability of the library.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the way we mocked the other tests should work just fine here, though!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cleaned it up a bit

routes/web.php Outdated

Route::post('/graphql', function (Request $request) {
$result = Utils::graphqlProxy($request->header(), $request->cookie(), $request->getContent());
return $result->getDecodedBody();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably want to return proxy call headers here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, check if the change I made is what you meant?

Route::post('/graphql', function (Request $request) {
    $result = Utils::graphqlProxy($request->header(), $request->cookie(), $request->getContent());
    return response($result->getDecodedBody())->withHeaders($result->getHeaders());
});

Base automatically changed from add-react-app-bridge to main May 11, 2021 19:08
Copy link
Collaborator

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing!

@vaimeo
Copy link

vaimeo commented Nov 9, 2022

I had following error when app is running on my server

Could not find session for GraphQL proxy

On localhost with ngrok it works fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants