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

Remove operation name from the regex and default to query #1004

Merged
merged 2 commits into from Jul 13, 2020

Conversation

jkimbo
Copy link
Member

@jkimbo jkimbo commented Jul 13, 2020

Fixes #1003

@jkimbo jkimbo requested a review from mvanlonden July 13, 2020 18:29
@eabruzzese
Copy link
Collaborator

@jkimbo this will break if there are multiple operations being sent. For example:

query FirstOperation {
  ...
}

subscription SecondOperation {
  ...
}

When you click the button to execute the operation, it'll have you choose the operationName from the dropdown. If you choose SecondOperation, the regex in this PR will match query, and won't send it over the websocket -- causing the operation to fail.

Let me take a look at that issue.

@jkimbo
Copy link
Member Author

jkimbo commented Jul 13, 2020

Good point @eabruzzese . Could check if operationName is defined and only use it in the regex if it is? Also I found this in the graphiql examples: https://github.com/graphql/graphiql/blob/main/examples/graphiql-cdn-subscriptions/index.html#L61-L74

But it requires the parse method which I assume comes from graphql. That might be a more robust option though.

@eabruzzese
Copy link
Collaborator

eabruzzese commented Jul 13, 2020

@jkimbo yeah, I saw that and tried to incorporate the parser, but couldn't get it working because of the format of the graphql CDN package (it doesn't have a UMD, and it also seemed like overkill for just detecting the operation type).

I like your idea there: if there's no operation name, just don't include it in the regex. The fallback to query is probably a good idea too. That should solve most cases for now. I'll try to incorporate the parser again tonight.

@eabruzzese
Copy link
Collaborator

@jkimbo I think you could just change graphQLParams.operationName + to (graphQLParams.operationName || '') + to get it working, yeah?

@eabruzzese
Copy link
Collaborator

@jkimbo Just tested with this branch. Works as expected.

@mvanlonden mvanlonden merged commit 63cfbbf into master Jul 13, 2020
@mvanlonden mvanlonden deleted the fix-graphiql-regex branch July 13, 2020 21:09
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.

GraphiQL not working in v2.12.0
3 participants