-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: implement persistedDocuments.hashAlgorithm
#9353
feat: implement persistedDocuments.hashAlgorithm
#9353
Conversation
…rsisted documents
🦋 Changeset detectedLatest commit: 79aae0a The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/presets/client/src/index.ts
Outdated
* @description Algorithm used to generate the hash, could be useful if your server expects something specific (e.g., Apollo Server expects `sha256`). | ||
* @default `sha1` | ||
*/ | ||
hashAlgorithm?: 'sha1' | 'sha256'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are more options accepted by crypto -- we can include them here and maybe even use a type from Crypto to type this (nevermind, it's typed as string in crypto anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crypto's algorithm
parameter is typed as a string
rather than a union because it solely depends on algorithms supported by the version of OpenSSL on the platform.
I don't know what is a proper type here, as in, what would be an appropriate subset of algorithms that are mainly supported everywhere. If we do something like 'sha1' | 'sha256' | string
, it will end up simplified as string
and we'll lose the ability to suggest.
I went down the path of only suggesting the default one sha1
as well as the "new" one sha256
that could be required by the server (e.g., Apollo Server). Consumers could still submit pull requests to extend the type here if something else is commonly used.
Thoughts? Open to feedback here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest typing it as: hashAlgorithm?: 'sha1' | 'sha256' | (string & {});
.
Should probably also extend the @description
to somehow mention this thing you said:
"Crypto's algorithm parameter is typed as a string rather than a union because it solely depends on algorithms supported by the version of OpenSSL on the platform." This should make the quite weird typing more understandable.
That way you keep the suggestions while still allowing users to step outside of the predefined algorithms.
If you then see that another algorithm is commonly used or requested, add it as one of the suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @ponbac. Good idea and a nice TS trick there!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clever! Will send a new commit for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, don't forget to update the generateDocumentHash
function signature as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 79aae0a, thanks for the good suggestions. 🙏
Can you please also document this on the docs? |
@n1ru4l |
Description
Proposal to add the ability to specify the hash algorithm used for persisted documents as mentioned by #9108.
Ideally, we would like the ability to pass our own
generateDocumentHash
function so consumers could use their own hash implementation. But, I figured we mostly want to comply with Apollo Server and/or still rely oncrypto
and therefore to avoid the dance withcrypto
:createHash
,update
, anddigest
, it should be more friendly to accept an algorithm directly.Related #9108
Type of change
How Has This Been Tested?
sha1
hashAlgorithm: 'sha256'
usessha256
hashAlgorithm: 'sha1'
usessha1
Checklist: