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

Add ability to specify custom document transform during manifest generation #412

Merged
merged 19 commits into from
Feb 22, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions .changeset/dirty-masks-nail.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
---
"@apollo/generate-persisted-query-manifest": minor
---

Add ability to specify a custom document transform used during manifest generation.

> [!NOTE]
> You must be running Apollo Client 3.8.0 or greater to use this feature.

> [!IMPORTANT]
> This should be the same document transform that is passed to your `ApolloClient` instance, otherwise you risk mismatches in the query output.

```ts
// persisted-query-manifest.config.ts
import { PersistedQueryManifestConfig } from "@apollo/generate-persisted-query-manifest";
import { DocumentTransform } from "@apollo/client/core";

const documentTransform = new DocumentTransform((document) => {
// transform your document
return transformedDocument;
})

const config: PersistedQueryManifestConfig = {
documentTransform,
};

export default config;
```
4 changes: 3 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion packages/generate-persisted-query-manifest/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,11 @@
"vfile-reporter": "^6.0.2"
},
"peerDependencies": {
"@apollo/client": "^3.7.0 || ^3.8.0-alpha",
"@apollo/client": "^3.7.0",
"graphql": "14.x || 15.x || 16.x"
},
"devDependencies": {
"@apollo/client": "3.9.5",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya good call! Wasn't sure how much I should clutter this PR up, but I think thats a good change.

Copy link
Member Author

Choose a reason for hiding this comment

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

"@gmrchk/cli-testing-library": "0.1.2",
"@wry/equality": "0.5.7"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { createHash } from "node:crypto";
import { readFileSync } from "node:fs";
import path from "node:path";
import type { PersistedQueryManifestOperation } from "../index";
import packageJson from "../../package.json";

test("prints help message with --help", async () => {
const { cleanup, runCommand } = await setup();
Expand Down Expand Up @@ -676,7 +677,7 @@ test("can specify custom document location with config file", async () => {

{
const ymlConfig = `
documents:
documents:
- queries/**/*.graphql
`;

Expand All @@ -690,7 +691,7 @@ module.exports = {
import { PersistedQueryManifestConfig } from '@apollo/generate-persisted-query-manifest';

const config: PersistedQueryManifestOperation = {
documents: ['queries/**/*.graphql']
documents: ['queries/**/*.graphql']
}

export default config;
Expand Down Expand Up @@ -894,7 +895,7 @@ const config: PersistedQueryManifestConfig = {
return Buffer.from(query).toString("base64");
default:
return createDefaultId();
}
}
}
};

Expand Down Expand Up @@ -925,6 +926,71 @@ export default config;
await cleanup();
});

test("can specify custom document transform in config", async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Are you going to do this? I'll approve but feel free to follow through on this idea or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes thanks! I'll add this before merging. Appreciate the approval!

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

const { cleanup, runCommand, writeFile, readFile, execute } = await setup();
const query = gql`
query GreetingQuery {
user {
id
name @custom
}
}
`;

const expectedQuery = gql`
query GreetingQuery {
user {
id
name
}
}
`;

await writeFile(
"./package.json",
JSON.stringify({
dependencies: {
"@apollo/client": packageJson.devDependencies["@apollo/client"],
},
}),
);
await execute("npm", "install");
await writeFile("./src/greeting-query.graphql", print(query));
await writeFile(
"./persisted-query-manifest.config.ts",
`
import { PersistedQueryManifestConfig } from '@apollo/generate-persisted-query-manifest';
import { DocumentTransform } from '@apollo/client/core';
import { removeDirectivesFromDocument } from '@apollo/client/utilities';

const documentTransform = new DocumentTransform((document) => {
return removeDirectivesFromDocument([{ name: 'custom' }], document);
});

const config: PersistedQueryManifestConfig = {
documentTransform,
};

export default config;
`,
);

const { code } = await runCommand();
const manifest = await readFile("./persisted-query-manifest.json");

expect(code).toBe(0);
expect(manifest).toBeManifestWithOperations([
{
id: sha256(addTypenameToDocument(expectedQuery)),
name: "GreetingQuery",
body: print(addTypenameToDocument(expectedQuery)),
type: "query",
},
]);

await cleanup();
});

test("errors on anonymous operations", async () => {
const { cleanup, runCommand, writeFile } = await setup();
const query = gql`
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
{
"extends": "../../../../tsconfig.test.base",
"include": ["**/*"],
"compilerOptions": {
"resolveJsonModule": true
},
"include": ["**/*", "../../package.json"],
"references": [{ "path": "../../" }]
}
26 changes: 25 additions & 1 deletion packages/generate-persisted-query-manifest/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ import {
InMemoryCache,
Observable,
} from "@apollo/client/core";
import type {
ApolloClientOptions,
DocumentTransform,
} from "@apollo/client/core";
import { sortTopLevelDefinitions } from "@apollo/persisted-query-lists";
import { gqlPluckFromCodeStringSync } from "@graphql-tools/graphql-tag-pluck";
import globby from "globby";
Expand Down Expand Up @@ -40,6 +44,21 @@ export interface PersistedQueryManifestConfig {
*/
documents?: string | string[];

/**
* A `DocumentTransform` instance that will be used to transform the GraphQL
* document before it is saved to the manifest.
*
* For more information about document transforms, see the [Document
* transforms](https://www.apollographql.com/docs/react/data/document-transforms)
* documentation page.
*
* IMPORTANT: You must be running `@apollo/client` 3.8.0 or greater to use
* this feature.
*
* @since 1.2.0
*/
documentTransform?: DocumentTransform;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


/**
* Path where the manifest file will be written.
*/
Expand Down Expand Up @@ -298,10 +317,15 @@ export async function generatePersistedQueryManifest(
...sources.map(({ node }) => node).filter(Boolean),
);
const manifestOperationIds = new Map<string, string>();

const manifestOperations: PersistedQueryManifestOperation[] = [];
const clientConfig: Partial<ApolloClientOptions<any>> = {};

if (config.documentTransform) {
clientConfig.documentTransform = config.documentTransform;
}

const client = new ApolloClient({
...clientConfig,
cache: new InMemoryCache({
fragments,
}),
Expand Down