-
-
Notifications
You must be signed in to change notification settings - Fork 451
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
createRequest implementation detail for comments (#) #1960
Comments
Hmm maybe something like this could solve our issue, https://regex101.com/r/D9Qpmd/1 this leverages negative lookbehind though so not entirely sure if this works in all browsers, would have to research. EDIT: seems like IE11 and Safari don't support it as noted here back to the drawing board 😅 |
I am really bad at regular expressions but I have an idea. What about something like this: let str = (typeof node !== 'string'
? (node.loc && node.loc.source.body) || print(node)
: node
)
.split("\n")
.map(line => line.trim().replace(/magic-regexp/g, ' ')) // regexp that does what it was doing previously but requires # to be at the start of the line?
.trim(); or even simpler solution - no regexp: let str = (typeof node !== 'string'
? (node.loc && node.loc.source.body) || print(node)
: node
)
.split('\n')
.filter(line => line.trim().indexOf('#') !== 0)
.join('\n')
.trim(); @edit: I guess my last solution (without regexes) won't work for cases where the comment is after a field selection so probably not ideal... |
Generally, I've personally known and ignored this because it's terribly rare to be a "valid" case. While it's possible and easy to target comments in GraphQL, it isn't a closed problem to target them in the grammar they live in. What the conundrum here was before is normalising queries quickly vs. normalising only their keys. So the question remains whether we continue to return the sanitised string in our keying function here: https://github.com/FormidableLabs/urql/blob/63eec7eb8b6efeb49c0bb27249dec58d12121d2a/packages/core/src/utils/request.ts#L53 The argument against this is ironically graphql-tag, which leads to our Either way, we could also opt to remove this normalisation from strings but Edit: also, it's worth noting, I'm pretty sure this behaviour is already isolated to its limit but the test is simply just not adhering to our expectations and definitions while being completely reasonably failing 😅 |
Well, I discovered/reported this bug because it broke my chat application in a really confusing way. When sending multiple messages that contained the same link with
followed by:
I guess it's worth noting that I am using graphql-zeus for creating type-safe queries. It's not using "placeholder variables" like you normally do when writing configurable queries but instead creates a new query with already replaced "placeholder variables" with their values. |
It's not magically replaced with the first one. Instead, even with comments, we seek to deduplicate operations that are essentially identical. A comment, for instance, doesn't change how a query executes and hence is irrelevant to its definition. Hence, the comments are stripped for the purpose of creating an operation key. What's surprising to me here is that a query is being created where the string is being embedded each time, for each run, rather than defining that as a reusable document with a variable. I am aware of GraphQL Zeus, but I neither agree that it's a great pattern to do this nor do I think that creating a JS DSL-like is better than a real DSL. That's of course debatable, but given that we'd have to strip strings entirely for a case that's otherwise rare that's just odd. Basically, I prefer the heuristic over vague edge cases. Because if we do aim to make this 100% accurate we'd basically have to parse the document entirely and stringify it in a consistent manner, which is simply too expensive for the purpose of creating a hash that's correct for all but some rare cases excluding those that don't use variables. I think it may make more sense to add a development-time warning for this, but just "dehancing" the normalisation isn't crucial in my eyes 😅 |
Any ideas what can be done to work around this issue besides monkey patching? I am considering using base64 |
You could use a regular interpolated equivalent instead 😅 const [,execute] = useMutation(`mutation ($input: String!) ...`);
execute({ input: 'hi#bye' }) I think you might be able to use: https://github.com/graphql-editor/graphql-zeus#variables |
Closing this one due to stale |
urql version & exchanges: master
Basically comment removal feature breaks the query hashing so the following test fails:
The text was updated successfully, but these errors were encountered: