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

feat: implement persistedDocuments.hashAlgorithm #9353

Conversation

charpeni
Copy link
Contributor

@charpeni charpeni commented Apr 27, 2023

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 on crypto and therefore to avoid the dance with crypto: createHash, update, and digest, it should be more friendly to accept an algorithm directly.

import { type CodegenConfig } from '@graphql-codegen/cli'
 
const config: CodegenConfig = {
  schema: 'schema.graphql',
  documents: ['src/**/*.tsx'],
  generates: {
    './src/gql/': {
      preset: 'client',
      presetConfig: {
-       persistedDocuments: true
+       persistedDocuments: {
+         hashAlgorithm: 'sha256',
+       },
      }
    }
  }
}

Related #9108

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Warning: There's no documentation related to persistedDocuments accepting an object instead of a boolean, outside of within the code. We should document those options (mode, hashPropertyName) and document hashAlgorithm at the same time.

How Has This Been Tested?

  • Still defaults to sha1
  • Passing hashAlgorithm: 'sha256' uses sha256
  • Passing hashAlgorithm: 'sha1' uses sha1
  • Added test

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@changeset-bot
Copy link

changeset-bot bot commented Apr 27, 2023

🦋 Changeset detected

Latest 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

@charpeni charpeni marked this pull request as ready for review April 27, 2023 19:04
@saihaj saihaj requested review from beerose and n1ru4l April 27, 2023 22:47
* @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';
Copy link
Contributor

@beerose beerose Apr 28, 2023

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)

Copy link
Contributor Author

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!

Copy link

@ponbac ponbac Apr 29, 2023

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.

Copy link
Contributor

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!

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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. 🙏

@n1ru4l
Copy link
Collaborator

n1ru4l commented Apr 28, 2023

Can you please also document this on the docs?

@charpeni
Copy link
Contributor Author

@n1ru4l persistedDocuments as a config object is currently not documented in the docs—at least, I couldn't find it. I plan on sending another pull request to document all the options of persistedDocuments including this new one.

@saihaj saihaj merged commit d7e335b into dotansimha:master May 9, 2023
@charpeni charpeni deleted the implement-persisted-documents-hash-algorithm branch May 9, 2023 13:41
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.

5 participants