Refactor createRequest parsing and hashing #383
Conversation
Co-authored-by: Jovi De Croock <[email protected]>
kitten
force-pushed
the
refactor/hashing-parsing-documents
branch
from
016f372 to
f63e197
Compare
| @@ -135,8 +135,6 @@ | |||
| }, | |||
| "dependencies": { | |||
| "fast-json-stable-stringify": "^2.0.0", | |||
| "graphql-tag": "^2.10.1", | |||
| "prop-types": "^15.6.0", | |||
There was a problem hiding this comment.
Glad we got rid of prop-types too!
There was a problem hiding this comment.
Yep, I've noticed we weren't using it and we probably just forgot to remove it
src/utils/request.ts
Outdated
| let key: number; | ||
| let query: DocumentNode; | ||
| if (typeof q === 'string') { | ||
| key = hash(q.replace(/[\s,]+/, ' ').trim()); |
There was a problem hiding this comment.
If I'm reading this correctly, this RegExp will just replace the first whitespace character or , it encounters (or both if they're adjacent) with a whitespace character. Can you explain a bit more what purpose that's serving? I'm reading through the hash code above to try to get more context but admittedly bitwise operators are not my strong suit .
There was a problem hiding this comment.
This is borrowed from graphql-tag itself and meant to stabilise hashes. Differently formatted queries will yield the same hash when the formatting is applied, so the intention is for two queries to have the same hash, even when they've been parsed & stringified or formatted through other means
There was a problem hiding this comment.
this RegExp will just replace the first whitespace character or
,it encounters
You're right, I misread your comment. Sorry! That was a copy&paste issue. I've just fixed it in the latest commit
|
This looks great! So this gets rid of GraphQL tag and also ensures that we have referential equality on document nodes with identical keys right? Also, what are the changes to the hash creation - different/optimized algo or something functionally different? |
|
@andyrichardson yep, this basically moves the logic that we've been relying on for parsing to our own codebase, so referential equality for two identically parsed queries is still guaranteed. We had some duplication around that with graphql-tag as well and it just didn't feel right to have that in the repo in a half-baked state while also relying on graphql-tag This will also have the added bonus of our bundle appearing to be smaller on bubdlephobia. This is of course not always guaranteed since some or most users will still rely on graphql-tag and pull that into their bundle. But since some are very interested in urql due to its attention to bundlesize, this will definitely help. The hashing method is the exact same. It's still using djb2 for its simplicity and speed. I've applied the same trick to it however that I have over on styled-components. The hashing function is split into the usual "hash" function and a "phash" (read: partial hash) function. This is a micro optimisation but essentially allows us to hash once and modify the hashed number with more string inputs. So that allows us to skip an extra step of stringifiying the query hash to add the variables hash on top. Edit: Sorry! Found a small inconsistency when hashing DocumentNodes vs strings. That's fixed and tested now. It's an uncommon edgecase though |
| if (typeof q === 'string') { | ||
| key = hashQuery(q); | ||
| query = docs[key] !== undefined ? docs[key] : parse(q); | ||
| } else if ((q as any)[keyProp] !== undefined) { |
There was a problem hiding this comment.
If I'm not mistaken, I believe we can get rid of all of these any type coercions. At this point in the branching, q should be correctly inferred as being of type DocumentNode.
There was a problem hiding this comment.
It is but the keyProp is our own property which is therefore not typed. I could of course cast it but casting to an interface here or any is essentially the same and doesn't make a difference as it's just for this function
Fix #377
Supersedes #380 by also refactoring hashing