Skip to content
This repository has been archived by the owner on Jun 29, 2021. It is now read-only.

add generateId option to graphql-mini-transforms #99

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

add generateId option to graphql-mini-transforms #99

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5439516
add generateId option to graphql-mini-transforms
dsanders11 Sep 29, 2020
7f42407
Apply suggestions from code review
dsanders11 Dec 14, 2020
d50b4da
Apply code review suggestion
dsanders11 Dec 14, 2020
fad6a45
Only minify source once when generating ID
dsanders11 Dec 14, 2020
f6d9023
Apply code review change
dsanders11 Dec 14, 2020
c8c54ff
address code review comments
dsanders11 Jan 6, 2021
13b8012
change generateId argument
dsanders11 Jan 6, 2021
d099346
fix linter error
dsanders11 Jan 6, 2021
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
25 changes: 24 additions & 1 deletion packages/graphql-mini-transforms/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ fragment ProductVariantId on ProductVariant {

#### Options

This loader accepts a single option, `simple`. This option changes the shape of the value exported from `.graphql` files. By default, a `graphql-typed` `DocumentNode` is exported, but when `simple` is set to `true`, a `SimpleDocument` is exported instead. This representation of GraphQL documents is smaller than a full `DocumentNode`, but generally won’t work with normalized GraphQL caches.
##### simple
dsanders11 marked this conversation as resolved.
Show resolved Hide resolved

This option changes the shape of the value exported from `.graphql` files. By default, a `graphql-typed` `DocumentNode` is exported, but when `simple` is set to `true`, a `SimpleDocument` is exported instead. This representation of GraphQL documents is smaller than a full `DocumentNode`, but generally won’t work with normalized GraphQL caches.

```js
module.exports = {
Expand All @@ -80,6 +82,27 @@ module.exports = {

If this option is set to `true`, you should also use the `jest-simple` transformer for Jest, and the `--export-format simple` flag for `graphql-typescript-definitions`.

##### generateId

This option changes the identifier value used. By default the hash of the minified GraphQL document is used as the identifier value, but when `generateId` is provided
the return value is used as the identifier value. `generateId` should be a function which takes a single parameter, the normalized GraphQL document source as a string,
dsanders11 marked this conversation as resolved.
Show resolved Hide resolved
and it should return a string value.

```js
module.exports = {
module: {
rules: [
{
test: /\.(graphql|gql)$/,
use: 'graphql-mini-transforms/webpack',
exclude: /node_modules/,
options: {generateId: normalizedSource => someHash(normalizedSource)},
},
],
},
};
```

### Jest

This package also provides a transformer for GraphQL files in Jest. To use the transformer, add a reference to it in your Jest configuration’s `transform` option:
Expand Down
4 changes: 3 additions & 1 deletion packages/graphql-mini-transforms/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"devDependencies": {
"@types/common-tags": "^1.8.0",
"@types/loader-utils": "^1.1.3",
"@types/schema-utils": "^2.4.0",
"common-tags": "^1.8.0"
},
"dependencies": {
Expand All @@ -43,6 +44,7 @@
"fs-extra": "^9.0.0",
"graphql": ">=14.5.0 <15.0.0",
"graphql-typed": "^0.6.1",
"loader-utils": "^2.0.0"
"loader-utils": "^2.0.0",
"schema-utils": "^2.7.1"
}
}
25 changes: 19 additions & 6 deletions packages/graphql-mini-transforms/src/document.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,21 @@ import {DocumentNode, SimpleDocument} from 'graphql-typed';
const IMPORT_REGEX = /^#import\s+['"]([^'"]*)['"];?[\s\n]*/gm;
const DEFAULT_NAME = 'Operation';

function defaultGenerateId(normalizedSource: string) {
// This ID is a hash of the full file contents that are part of the document,
// including other documents that are injected in, but excluding any unused
// fragments. This is useful for things like persisted queries.
return createHash('sha256').update(normalizedSource).digest('hex');
}

export interface CleanDocumentOptions {
removeUnused?: boolean;
generateId?: (normalizedSource: string) => string;
}

export function cleanDocument(
document: UntypedDocumentNode,
{removeUnused = true} = {},
{removeUnused = true, generateId}: CleanDocumentOptions = {},
): DocumentNode<any, any, any> {
if (removeUnused) {
removeUnusedDefinitions(document);
Expand All @@ -28,17 +40,18 @@ export function cleanDocument(
addTypename(definition);
}

const normalizedSource = minifySource(print(document));
const documentSource = print(document);
const normalizedSource = minifySource(documentSource);
const normalizedDocument = parse(normalizedSource);

for (const definition of normalizedDocument.definitions) {
stripLoc(definition);
}

// This ID is a hash of the full file contents that are part of the document,
// including other documents that are injected in, but excluding any unused
// fragments. This is useful for things like persisted queries.
const id = createHash('sha256').update(normalizedSource).digest('hex');
const id =
generateId === undefined
Copy link
Member

Choose a reason for hiding this comment

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

Instead of checking this here, can you add a default value when destructuring the argument?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I follow this remark. What default value are you suggesting? The line you've commented on would still be required, so do you just want to explicitly destructure to undefined or null?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think what he proposed is something like :

{removeUnused = true, generateId = defaultGenerateId}: CleanDocumentOptions = {},
...
const id = generateId(normalizedSource)

If we want to do something like this, both defaultGenerateId and generateId will have to receive the same source. Currently, defaultGenerateId receives the normalized source and the generateId receives the document source.

I don't remember why generateId needs the document source.

Copy link
Author

Choose a reason for hiding this comment

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

Well, that's what the code originally looked like... the commits and comments leading to that change are still there for review. That's why I'm a bit confused.

While changing the code to prevent printing the source twice, I also changed it so that minifySource was only called once since the extra operations seemed to be a concern.

The general generateId needs the unminifed source (this is explained in a previous comment). The default implementation minifies it, so it was happening twice. That motivated that change.

I can change it back, but I'm going to address the other comments first and leave this for now, so I don't go in circles on this.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry if I just missed this, but I am a bit confused why generateId needs to take the "full" document source, not the minified version. I assumed everything should be operating on the minified source, since that includes all the relevant fragments and normalizes away differences in whitespace and commas.

Copy link
Contributor

@alexandcote alexandcote Jan 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think we need to take a decision on what we want here. Do we want to pass the minified version or the "full" document source to the generateId function?

  • Passing the minified version to the generateId would be the simplest solution but I think this was causing an issue for your use case @dsanders11 ?
  • Passing the "full" document to both functions will result in minifying twice the document (not sure of the cost of this. It was the first implementation)
  • Having the current implementation solves @dsanders11 limitation and ensures we only minify the source once but the API is not constant.

The key points here are what is the limitation with the minified version @dsanders11 and does the cost of the minification is high @lemonmade?

Copy link
Author

Choose a reason for hiding this comment

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

@alexandcote, I think you've accurately summarized where things stand.

  • Passing the minified version to the generateId would be the simplest solution but I think this was causing an issue for your use case @dsanders11 ?

In no way trying to be rude, but all of the previous discussion and explanation still exists in this PR, nothing has been deleted. If you don't remember something about certain choices I'd suggest looking back on the other comments here for clarification. My comment from September explains why the non-minified version is needed for generateId. If that explanation isn't clear, I can try to clarify any point of confusion. The TL;DR is simply that the string given to generateId should match the query sent to the server, and the query sent to the server is the non-minified version.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, this is an unfortunate side effect of me/ us having let a PR review go for 4 months :p

I see where you are coming from on this, but I worry that the approach is very specific to one very special case where the printed, "normalized" document is identical to the original GraphQL source. This only happens when you aren't using fragments from external files, and you aren't letting the library automatically add __typename fields. IMO, this case is specific and uncommon enough (__typename additions are on by default), that we should push the pain of dealing with it to consumers' application code. An application can fairly easily run the same minification on the server source before computing the SHA (happy to export a utility for it if necessary), and that will work regardless of the fragment/ typename situation (and also works if you ever switch to our "simple" document format that doesn't need to be parsed and re-printed by the GraphQL client).

How do you feel about that?

Copy link
Member

Choose a reason for hiding this comment

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

If we don't do that, I think we need to give the "generate me an ID" option three different sources that might be relevant for them: the minified source code, the original document source code (no fragments or typenames embedded), and the "normalized" document that includes all the fragments and typenames.

? defaultGenerateId(normalizedSource)
: generateId(documentSource);

Reflect.defineProperty(normalizedDocument, 'id', {
value: id,
Expand Down
21 changes: 19 additions & 2 deletions packages/graphql-mini-transforms/src/webpack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,37 @@ import {dirname} from 'path';
import {loader} from 'webpack';
import {parse, DocumentNode} from 'graphql';
import {getOptions} from 'loader-utils';
import validateOptions from 'schema-utils';

import {cleanDocument, extractImports, toSimpleDocument} from './document';

interface Options {
generateId?: (normalizedSource: string) => string;
dsanders11 marked this conversation as resolved.
Show resolved Hide resolved
simple?: boolean;
}

const schema = {
type: 'object' as const,
properties: {
simple: {
type: 'boolean' as const,
},
generateId: {
instanceof: 'Function' as const,
},
},
};

export default async function graphQLLoader(
this: loader.LoaderContext,
source: string | Buffer,
) {
this.cacheable();

const done = this.async();
const {simple = false} = getOptions(this) as Options;
const options: Options = {simple: false, ...getOptions(this)};

validateOptions(schema, options, {name: '@shopify/graphql-mini-transforms'});

if (done == null) {
throw new Error(
Expand All @@ -28,8 +44,9 @@ export default async function graphQLLoader(
try {
const document = cleanDocument(
await loadDocument(source, this.context, this),
{generateId: options.generateId},
);
const exported = simple ? toSimpleDocument(document) : document;
const exported = options.simple ? toSimpleDocument(document) : document;

done(
null,
Expand Down
8 changes: 8 additions & 0 deletions packages/graphql-mini-transforms/tests/webpack.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,14 @@ describe('graphql-mini-transforms/webpack', () => {
);
});

it('has option for custom ID generate function', async () => {
dsanders11 marked this conversation as resolved.
Show resolved Hide resolved
const result = await extractDocumentExport(
`query Shop { shop { id } }`,
createLoaderContext({query: {generateId: () => 'foo'}}),
);
expect(result).toHaveProperty('id', 'foo');
dsanders11 marked this conversation as resolved.
Show resolved Hide resolved
});

describe('import', () => {
it('adds the resolved import as a dependency', async () => {
const context = '/app/';
Expand Down
36 changes: 36 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,11 @@
resolved "https://registry.yarnpkg.com/@types/json-schema/-/json-schema-7.0.4.tgz#38fd73ddfd9b55abb1e1b2ed578cb55bd7b7d339"
integrity sha512-8+KAKzEvSUdeo+kmqnKrqgeE+LcA0tjYWFY7RPProVYwnqDjukzO+3b6dLD56rYX5TdWejnEOLJYOIeh4CXKuA==

"@types/json-schema@^7.0.5":
version "7.0.6"
resolved "https://registry.yarnpkg.com/@types/json-schema/-/json-schema-7.0.6.tgz#f4c7ec43e81b319a9815115031709f26987891f0"
integrity sha512-3c+yGKvVP5Y9TYBEibGNR+kLtijnj7mYrXRg+WpFb2X9xm04g/DXYkfg4hmzJQosc9snFNUPkbYIhu+KAm6jJw==

"@types/loader-utils@^1.1.3":
version "1.1.3"
resolved "https://registry.yarnpkg.com/@types/loader-utils/-/loader-utils-1.1.3.tgz#82b9163f2ead596c68a8c03e450fbd6e089df401"
Expand Down Expand Up @@ -751,6 +756,13 @@
resolved "https://registry.yarnpkg.com/@types/prettier/-/prettier-1.19.1.tgz#33509849f8e679e4add158959fdb086440e9553f"
integrity sha512-5qOlnZscTn4xxM5MeGXAMOsIOIKIbh9e85zJWfBRVPlRMEVawzoPhINYbRGkBZCI8LxvBe7tJCdWiarA99OZfQ==

"@types/schema-utils@^2.4.0":
version "2.4.0"
resolved "https://registry.yarnpkg.com/@types/schema-utils/-/schema-utils-2.4.0.tgz#9983012045d541dcee053e685a27c9c87c840fcd"
integrity sha512-454hrj5gz/FXcUE20ygfEiN4DxZ1sprUo0V1gqIqkNZ/CzoEzAZEll2uxMsuyz6BYjiQan4Aa65xbTemfzW9hQ==
dependencies:
schema-utils "*"

"@types/source-list-map@*":
version "0.1.2"
resolved "https://registry.yarnpkg.com/@types/source-list-map/-/source-list-map-0.1.2.tgz#0078836063ffaf17412349bba364087e0ac02ec9"
Expand Down Expand Up @@ -909,6 +921,11 @@ add-stream@^1.0.0:
resolved "https://registry.yarnpkg.com/add-stream/-/add-stream-1.0.0.tgz#6a7990437ca736d5e1288db92bd3266d5f5cb2aa"
integrity sha1-anmQQ3ynNtXhKI25K9MmbV9csqo=

ajv-keywords@^3.5.2:
version "3.5.2"
resolved "https://registry.yarnpkg.com/ajv-keywords/-/ajv-keywords-3.5.2.tgz#31f29da5ab6e00d1c2d329acf7b5929614d5014d"
integrity sha512-5p6WTN0DdTGVQk6VjcEju19IgaHudalcfabD7yhDGeA6bcQnmL+CpveLJq/3hvfwd1aof6L386Ougkx6RfyMIQ==

ajv@^6.10.0, ajv@^6.10.2, ajv@^6.5.5:
version "6.12.0"
resolved "https://registry.yarnpkg.com/ajv/-/ajv-6.12.0.tgz#06d60b96d87b8454a5adaba86e7854da629db4b7"
Expand All @@ -919,6 +936,16 @@ ajv@^6.10.0, ajv@^6.10.2, ajv@^6.5.5:
json-schema-traverse "^0.4.1"
uri-js "^4.2.2"

ajv@^6.12.4:
version "6.12.5"
resolved "https://registry.yarnpkg.com/ajv/-/ajv-6.12.5.tgz#19b0e8bae8f476e5ba666300387775fb1a00a4da"
integrity sha512-lRF8RORchjpKG50/WFf8xmg7sgCLFiYNNnqdKflk63whMQcWR5ngGjiSXkL9bjxy6B2npOK2HSMN49jEBMSkag==
dependencies:
fast-deep-equal "^3.1.1"
fast-json-stable-stringify "^2.0.0"
json-schema-traverse "^0.4.1"
uri-js "^4.2.2"

ansi-escapes@^3.0.0:
version "3.2.0"
resolved "https://registry.yarnpkg.com/ansi-escapes/-/ansi-escapes-3.2.0.tgz#8780b98ff9dbf5638152d1f1fe5c1d7b4442976b"
Expand Down Expand Up @@ -5884,6 +5911,15 @@ saxes@^3.1.9:
dependencies:
xmlchars "^2.1.1"

schema-utils@*, schema-utils@^2.7.1:
version "2.7.1"
resolved "https://registry.yarnpkg.com/schema-utils/-/schema-utils-2.7.1.tgz#1ca4f32d1b24c590c203b8e7a50bf0ea4cd394d7"
integrity sha512-SHiNtMOUGWBQJwzISiVYKu82GiV4QYGePp3odlY1tuKO7gPtphAT5R/py0fA6xtbgLL/RvtJZnU9b8s0F1q0Xg==
dependencies:
"@types/json-schema" "^7.0.5"
ajv "^6.12.4"
ajv-keywords "^3.5.2"

"semver@2 || 3 || 4 || 5", semver@^5.1.0, semver@^5.4.1, semver@^5.5.0:
version "5.7.1"
resolved "https://registry.yarnpkg.com/semver/-/semver-5.7.1.tgz#a954f931aeba508d307bbf069eff0c01c96116f7"
Expand Down