Add ability to specify custom document transform during manifest generation #412
Conversation
🦋 Changeset detectedLatest commit: 6a57af8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| @@ -40,6 +40,7 @@ | |||
| "graphql": "14.x || 15.x || 16.x" | |||
| }, | |||
| "devDependencies": { | |||
| "@apollo/client": "3.9.5", | |||
There was a problem hiding this comment.
should we update the peer dep while we're at it? maybe we don't really need to leave in support for a random alpha now that 3.8.0 is old news
There was a problem hiding this comment.
Ya good call! Wasn't sure how much I should clutter this PR up, but I think thats a good change.
| * transforms](https://www.apollographql.com/docs/react/data/document-transforms) | ||
| * documentation page. | ||
| */ | ||
| documentTransform?: DocumentTransform; |
There was a problem hiding this comment.
Is this the best approach or should we just have a clientConfig: Partial<ApolloClientOptions<any>> field? Maybe Omitting link and cache, though we should probably also have a way to configure the cache?
There was a problem hiding this comment.
Thats not a bad idea. It would future proof it a bit more and could avoid a situation where you're using Apollo Client v3.7 where the documentTransform option isn't available. In that case, the client options should reflect the version you're on.
But you're right that we probably should omit a few options that don't make sense (I can also think of the uri option for example). Let me take a look at the full client options list and see what makes sense to omit.
There was a problem hiding this comment.
Looking a bit more through the client options I'd actually say that almost all of these are unnecessary for manifest generation since a lot of them are for actual network calls. Let's talk through these in our chat later and figure out how we feel about it.
There was a problem hiding this comment.
FWIW I didn't mean to omit them because they don't make sense, but rather because they're what we explicitly pass in ourselves.
There was a problem hiding this comment.
We talked on Zoom and I agree that there aren't any other compelling options other than documentTransform so we might as well just do it this way.
| @@ -925,6 +926,71 @@ export default config; | |||
| await cleanup(); | |||
| }); | |||
|
|
|||
| test("can specify custom document transform in config", async () => { | |||
There was a problem hiding this comment.
Should probably add a test for Apollo Client 3.7 since document transforms aren't run. Will do this after we figure out the right approach.
There was a problem hiding this comment.
Are you going to do this? I'll approve but feel free to follow through on this idea or not.
There was a problem hiding this comment.
Ah yes thanks! I'll add this before merging. Appreciate the approval!
There was a problem hiding this comment.
Turns out this is a bit tougher than I originally thought because its difficult to control module resolution on the CLI package itself in tests. While I can install Apollo Client 3.7 which is properly used in the config itself, the cli still resolves to the latest version of the client, so the transform is still run.
I'll test locally with a built package against a repo with 3.7 just to make sure I didn't miss anything, but I feel pretty confident this is ok otherwise.
There was a problem hiding this comment.
Added an update to provide a nicer type when using Apollo Client 3.7 where DocumentTransform is not available: d7475fa. Now when trying to use that option when that version is installed, the only valid value is undefined.
|
Ping me when the README is ready. Looks like this doesn't solve the particular onboarding use case we thought it might, but it's still a great idea. |
|
@glasser README is updated! |
| @@ -925,6 +926,71 @@ export default config; | |||
| await cleanup(); | |||
| }); | |||
|
|
|||
| test("can specify custom document transform in config", async () => { | |||
There was a problem hiding this comment.
Are you going to do this? I'll approve but feel free to follow through on this idea or not.
… Apollo Client version
Apollo Client v3.8 added support for custom document transforms that can be used to modify queries before they are sent to the server. This PR adds support for this feature during manifest generation to ensure the queries saved to the manifest match what will be sent by the client in a running app.