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

Migrate apollo-link-persisted-queries functionality into Apollo Client #6837

Merged
merged 15 commits into from
Sep 11, 2020

Conversation

hwillson
Copy link
Member

@hwillson hwillson commented Aug 14, 2020

This PR migrates the https://github.com/apollographql/apollo-link-persisted-queries code into this repo, updating it to work with Apollo Client 3 (based on the changes in apollographql/apollo-link-persisted-queries#48). This will allow us to maintain the persisted queries link alongside our other supported links, from the new package entry point of @apollo/client/link/persisted-queries.

As part of this migration, we're looking into a better way to provide sha256 hashing capabilities (across Node and browsers) without requiring a relatively heavy dependency like hash.js. This PR introduces the use of crypto-hash to leverage Node's built-in crypto capabilities and browser's built-in window.crypto API. This approach is not set in stone, and is part of this PR to kickstart further discussion. Leveraging built-in Node/browser functionality is of course a big win with regards to bundle sizes (e.g. crypto-hash introduces a small 508B min+gz to the project), but introduces drawbacks with regards to browser support. For example, use of crypto-hash means using browser TextEncoder and SubtleCrypto.digest API's, which are supported by most browsers, but IE11 could be a problem. To accommodate this we could either recommend polyfilling when using IE11, or consider providing built-in fallback hashing functionality (it would be nice to avoid adding in more code and leverage built-in API's if possible though).

Since this code was migrated from the apollo-link-persisted-queries repo, a lot of it is as it was. I'll try to highlight the main areas I've touched as part of this PR, to help guide review.

Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed on Slack, it would be great if this implementation could accept any (possibly asynchronous) hash implementation, since the automatic persisted queries protocol does not depend on any specific hash function. Then we could provide guidance in the docs about how to provide a reasonable sha256 implementation, so folks could make their own decisions about browser compatibility and code size (specifically, while crypto-hash is small, it ships with untranspiled async/await syntax, so it only works in modern browsers, which might be perfectly acceptable for some projects, but probably not for everyone).

@hwillson hwillson force-pushed the persisted-queries-link branch 4 times, most recently from f3e268e to 3a499b6 Compare August 24, 2020 12:08
@hwillson
Copy link
Member Author

@benjamn quick FYI - I’ve finished taking a pass at the technical adjustments we discussed, but haven’t brought this out of draft yet as I think we’ll need to get a new persisted queries links section and content into the AC docs. I’ll have this done shortly.

@benjamn benjamn modified the milestones: Release 2.x, Post 3.0 Sep 8, 2020
@hwillson hwillson force-pushed the persisted-queries-link branch from ae00e04 to f975903 Compare September 9, 2020 15:01
@jeffreyffs
Copy link

Been following the apollo client 3 upgrade for a while. My project is blocked from upgrading because we are using persisted queries. Is there any workaround around to getting persisted queries to work with apollo client 3? Or do we need to wait for this to be released? If so, is there an ETA? Thanks!

@hwillson hwillson marked this pull request as ready for review September 10, 2020 17:49
@hwillson
Copy link
Member Author

@benjamn @jcreighton This is now ready for review (I haven't added anything to the CHANGELOG yet, since this isn't going to land in 3.2). Thanks!

@hwillson hwillson requested a review from benjamn September 10, 2020 17:50
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed some commits that I thought were worthwhile, but overall I'm happy with this. Thanks for pulling this together @hwillson!

benjamn added a commit that referenced this pull request Sep 11, 2020
@@ -34,6 +34,7 @@ export default function transformer(file, api) {
'batch-http',
'context',
'error',
'persisted-queries',
Copy link
Member

@benjamn benjamn Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super easy, thanks to the work we (i.e. @jcreighton) did in #6486!

hwillson and others added 14 commits September 11, 2020 14:31
This code was migrated from the `apollo-link-persisted-queries`
repo, and modified to work with Apollo Client 3.
We've decided to avoid falling back in a particular SHA-256
function by default, to avoid bundling one with Apollo Client.
`crypto-hash` is now only using for testing.
We're no longer providing a SHA-256 function by default, so these
changes ensure that an appropriate function is provided by
developers. Developers can choose to supply a SHA-256 function
via the `sha256` option, if they want the default hash generation
capabilities of the Link (e.g. sha256(print(query))), or they
can override the hashing process completely by supplying a
custom `generateHash` function.
These changes also ensure that the tests cover supplying a
sync or async SHA-256 function.
benjamn added a commit that referenced this pull request Sep 11, 2020
@benjamn benjamn force-pushed the persisted-queries-link branch from 7439c9e to 65e60d3 Compare September 11, 2020 18:32
@benjamn benjamn force-pushed the persisted-queries-link branch from 65e60d3 to 701eb3b Compare September 11, 2020 18:36
@benjamn benjamn merged commit f2b60c5 into release-3.2 Sep 11, 2020
@benjamn benjamn deleted the persisted-queries-link branch September 11, 2020 18:39
@benjamn benjamn mentioned this pull request Sep 11, 2020
11 tasks
@jeffreyffs
Copy link

It made it into 3.2! Awesome. Excited to try it out 🙇

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants