add generateId option to graphql-mini-transforms #99
Conversation
dsanders11
force-pushed
the
graphql-mini-transforms-generateId
branch
from
33153ed to
d76ff38
Compare
|
@lemonmade, thanks for taking a look. I'll try to get this PR updated with your requested changes when I get some time. In the meanwhile, I'm going to respond to any comments which need clarification. |
dsanders11
force-pushed
the
graphql-mini-transforms-generateId
branch
from
d76ff38 to
4683827
Compare
|
@lemonmade, this has now been updated to be based on the latest |
dsanders11
force-pushed
the
graphql-mini-transforms-generateId
branch
from
4683827 to
5439516
Compare
|
Rebased onto latest @lemonmade @alexandcote think you could take a look at this PR? Would be nice to get it merged so it doesn't keep slipping into a conflicted state. |
There was a problem hiding this comment.
Hi @dsanders11
First of all, thanks a lot for your contribution. I reviewed your PR, nothing major only nit 💅.
Now that I'm following this PR, feel free to ping me again when you have time for the small tweaks. If you know you will not have time to do the small changes, let me know I can probably take time before the holiday.
Again, thanks a lot for your contribution and sorry for the long delay on this one 😥 ...
Co-authored-by: Alexandre Côté <[email protected]>
dsanders11
force-pushed
the
graphql-mini-transforms-generateId
branch
from
7018c8d to
7f42407
Compare
There was a problem hiding this comment.
This PR looks good to me ✅ . I will follow with @lemonmade tomorrow. Thanks for your quick rework 💯
|
@alexandcote, I think I snuck in one more commit while you were making that comment, fad6a45. Per your comment about So that's just a little further optimization, but I'm also fine to drop that commit. Just thought I'd point it out. Thanks for taking a look and the quick feedback. |
dsanders11
force-pushed
the
graphql-mini-transforms-generateId
branch
from
a7bc4a3 to
f6d9023
Compare
|
Alright, changes made, I'll stop touching it so it can be merged. 🙂 |
There was a problem hiding this comment.
@alexandcote had asked me to take a look. And it all looks great!
|
I re-request a review from @lemonmade. Probably this will have to wait until after the holiday tho. |
| // fragments. This is useful for things like persisted queries. | ||
| const id = createHash('sha256').update(normalizedSource).digest('hex'); | ||
| const id = | ||
| generateId === undefined |
There was a problem hiding this comment.
Instead of checking this here, can you add a default value when destructuring the argument?
There was a problem hiding this comment.
I'm not sure I follow this remark. What default value are you suggesting? The line you've commented on would still be required, so do you just want to explicitly destructure to undefined or null?
There was a problem hiding this comment.
I think what he proposed is something like :
{removeUnused = true, generateId = defaultGenerateId}: CleanDocumentOptions = {},
...
const id = generateId(normalizedSource)If we want to do something like this, both defaultGenerateId and generateId will have to receive the same source. Currently, defaultGenerateId receives the normalized source and the generateId receives the document source.
I don't remember why generateId needs the document source.
There was a problem hiding this comment.
Well, that's what the code originally looked like... the commits and comments leading to that change are still there for review. That's why I'm a bit confused.
While changing the code to prevent printing the source twice, I also changed it so that minifySource was only called once since the extra operations seemed to be a concern.
The general generateId needs the unminifed source (this is explained in a previous comment). The default implementation minifies it, so it was happening twice. That motivated that change.
I can change it back, but I'm going to address the other comments first and leave this for now, so I don't go in circles on this.
There was a problem hiding this comment.
Sorry if I just missed this, but I am a bit confused why generateId needs to take the "full" document source, not the minified version. I assumed everything should be operating on the minified source, since that includes all the relevant fragments and normalizes away differences in whitespace and commas.
There was a problem hiding this comment.
I think we need to take a decision on what we want here. Do we want to pass the minified version or the "full" document source to the generateId function?
- Passing the minified version to the
generateIdwould be the simplest solution but I think this was causing an issue for your use case @dsanders11 ? - Passing the "full" document to both functions will result in minifying twice the document (not sure of the cost of this. It was the first implementation)
- Having the current implementation solves @dsanders11 limitation and ensures we only minify the source once but the API is not constant.
The key points here are what is the limitation with the minified version @dsanders11 and does the cost of the minification is high @lemonmade?
There was a problem hiding this comment.
@alexandcote, I think you've accurately summarized where things stand.
- Passing the minified version to the generateId would be the simplest solution but I think this was causing an issue for your use case @dsanders11 ?
In no way trying to be rude, but all of the previous discussion and explanation still exists in this PR, nothing has been deleted. If you don't remember something about certain choices I'd suggest looking back on the other comments here for clarification. My comment from September explains why the non-minified version is needed for generateId. If that explanation isn't clear, I can try to clarify any point of confusion. The TL;DR is simply that the string given to generateId should match the query sent to the server, and the query sent to the server is the non-minified version.
There was a problem hiding this comment.
Sorry, this is an unfortunate side effect of me/ us having let a PR review go for 4 months :p
I see where you are coming from on this, but I worry that the approach is very specific to one very special case where the printed, "normalized" document is identical to the original GraphQL source. This only happens when you aren't using fragments from external files, and you aren't letting the library automatically add __typename fields. IMO, this case is specific and uncommon enough (__typename additions are on by default), that we should push the pain of dealing with it to consumers' application code. An application can fairly easily run the same minification on the server source before computing the SHA (happy to export a utility for it if necessary), and that will work regardless of the fragment/ typename situation (and also works if you ever switch to our "simple" document format that doesn't need to be parsed and re-printed by the GraphQL client).
How do you feel about that?
There was a problem hiding this comment.
If we don't do that, I think we need to give the "generate me an ID" option three different sources that might be relevant for them: the minified source code, the original document source code (no fragments or typenames embedded), and the "normalized" document that includes all the fragments and typenames.
| // fragments. This is useful for things like persisted queries. | ||
| const id = createHash('sha256').update(normalizedSource).digest('hex'); | ||
| const id = | ||
| generateId === undefined |
There was a problem hiding this comment.
Sorry if I just missed this, but I am a bit confused why generateId needs to take the "full" document source, not the minified version. I assumed everything should be operating on the minified source, since that includes all the relevant fragments and normalizes away differences in whitespace and commas.
|
👋 So sorry for the trouble, we have move this package to https://github.com/Shopify/quilt/tree/main/packages/graphql-mini-transforms please move this PR over to quilt if you would still like to contribute 🙇♀️ |
Effectively the same as the
generateIdoption fromgraphql-persisted-document-loader.I've got a use-case where I'd like to make the ID a signature of the hash, rather than just the hash, so that the server can trust queries presented by the client, since the IDs were signed at build-time. Removes the need to distribute a generated mapping like that from
@shopify/webpack-persisted-graphql-plugin.Being able to customize the generated ID would make this possible, and it's a pretty small change.