The deduplicator now accepts a query param #312

lfades · 2018-05-11T15:43:43Z

Custom headers are deprecated (also read this issue).

This is not a breaking change, a header will still work but now the docs show the usage with the query param ?deduplicate instead.

As a side note: I ran the script format and some code that's not related to this change was modified according to Prettier rules

…to improve-tests

kachkaev · 2018-05-11T15:50:03Z

README.md

@@ -128,7 +128,7 @@ The `options` object has the following fields:
 | `uploads`       | [UploadOptions](/src/types.ts#L39-L43) or `false` or `undefined` | `null`          | Provides information about upload limits; the object can have any combination of the following three keys: `maxFieldSize`, `maxFileSize`, `maxFiles`; each of these have values of type Number; setting to `false` disables file uploading                                                                                   |
 | `https`         | [HttpsOptions](/src/types.ts#L62-L65) or `undefined`             | `undefined`     | Enables HTTPS support with a key/cert
 | `getEndpoint`  | String or Boolean |  `false`  | Adds a graphql HTTP GET endpoint to your server (defaults to `endpoint` if `true`).  Used for leveraging CDN level caching. |
-| `deduplicator` | Boolean | `true` | Enables [graphql-deduplicator](https://github.com/gajus/graphql-deduplicator). Once enabled sending the header `X-GraphQL-Deduplicate` will deduplicate the data.  |
+| `deduplicator` | Boolean | `true` | Enables [graphql-deduplicator](https://github.com/gajus/graphql-deduplicator). Once enabled sending the query param `?deduplicate` will deduplicate the data.  |


How about this?

Once enabled, adding ?deduplicate to endpoint URL will deduplicate the data.

kachkaev · 2018-05-11T15:52:09Z

src/index.test.ts

+    body: { query },
+  }).promise()
+
+  const deduplicatedWithHeader = await request({


It might be useful to state why the test is here. Others might be surprised seeing it despite that the docs don't mention X-GraphQL-Deduplicate.

Right, a comment can make it more clear

kachkaev · 2018-05-11T15:56:40Z

src/index.ts

@@ -185,7 +185,8 @@ export class GraphQLServer {
      }
      return (response, ...args) => {
        if (
-          req.get('X-GraphQL-Deduplicate') &&
+          (req.get('X-GraphQL-Deduplicate') ||
+            req.query.deduplicate !== undefined) &&


I'd check against req.query.deduplicate first given that this is now a preferred method. Perhaps, a comment about the legacy support of X-GraphQL-Deduplicate can be helpful too.

I'm also wondering if there is a cheap way to be a bit smarter with handling the value. It'd be great to consider ?deduplicate, ?deduplicate=true and ?deduplicate=1 as true, while treat any other value as false. I'd be also great if this check could be later reused for graphql-crunch too (#303).

I'd like such restriction for other values and it should be very simple to check.

kachkaev · 2018-05-11T16:02:41Z

src/index.test.ts

  })

+  t.deepEqual(deduplicated, deduplicatedWithHeader)
+
  t.deepEqual(body.data, inflate(deduplicated.data))


If neither of deduplications worked, will the test pass?

No, that one is just to check that both are the same, to check that data is properly deduplicated there's another check above

Ah, sure! I somehow missed t.deepEqual above 😅

kachkaev · 2018-05-11T19:19:39Z

src/index.ts

@@ -184,8 +183,11 @@ export class GraphQLServer {
        return this.options.formatResponse
      }
      return (response, ...args) => {
+        const truthyValues = ['', '1', 'true']


Could this array be initialised once rather than at every request? It'd be probably useful to name it a bit more explicitly if moving elsewhere, say to trulyValuesForQueryParameter. I'm also wondering if '' would be the value for ?deduplicate just like in the case of ?deduplicate=. Can't check this now, but could the first case produce null? I'm also not sure that ?deduplicate= should be treated as true, to me this is like ?deduplicate=false.

It'd be nice to add a test with a non-truly value (e.g. ?deduplicate=false) to make sure we're not accidentally deduplicating at all times. If we turn a check into a function, it may be easier to unit-test and reuse with ?crunch in future.

?deduplicate and ?deduplicate= are the same (''), I think we can treat both as true or only allow ?deduplicate=true and ?deduplicate=1, otherwise we'll need to do another check

Basically we can do something like this:

function isTrulyQueryParam(value: string) { return value === '1' || value === 'true' }

I'd go for treating ?deduplicate, ?deduplicate=, ?deduplicate=true and ?deduplicate=1 as true then. I guess people would want to use http://example.com/graphql?deduplicate – it looks pretty clean.

/** * Normalises GET param values into a boolean. * Returns true in the following cases: * ?param ?param= ?param=1 ?param=true */ function isTrulyQueryParam(value: string) { return value === '' || value === '1' || value === 'true' }

lfades · 2018-05-18T16:32:02Z

This PR is ready to be merged now 💃

kachkaev · 2018-06-07T20:16:20Z

ping @timsuchanek / @schickling 😉

stale · 2018-11-23T03:09:55Z

Due to inactivity of this issue we have marked it stale. It will be closed if no further activity occurs.

stale · 2018-11-30T03:42:33Z

Hey 👋, It seems like this issue has been inactive for some time. In need for maintaining clear overview of the issues concerning the latest version of graphql-yoga we'll close it.
Feel free to reopen it at any time if you believe we should futher discuss its content. 🙂

lfades added 3 commits May 11, 2018 10:28

the deduplicator now accepts a query param and code prettified

07a1be2

Now the docs use the query param for the deduplicate option

878bce8

Merge branch 'master' of https://github.com/graphcool/graphql-yoga in…

554b5bc

…to improve-tests

kachkaev reviewed May 11, 2018

View reviewed changes

lfades added 2 commits May 11, 2018 12:53

Improved docs for the deduplicator option

02f865c

Now only some values are taken as valid for ?deduplicate

e07b8f2

kachkaev reviewed May 11, 2018

View reviewed changes

lfades added 2 commits May 18, 2018 11:26

Some changes to formatResponse

016cd0e

Merged with master

6c60de9

stale bot added the stale label Nov 23, 2018

stale bot closed this Nov 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The deduplicator now accepts a query param #312

The deduplicator now accepts a query param #312

lfades commented May 11, 2018

kachkaev May 11, 2018

kachkaev May 11, 2018 •

edited

Loading

lfades May 11, 2018

kachkaev May 11, 2018 •

edited

Loading

lfades May 11, 2018

kachkaev May 11, 2018

lfades May 11, 2018

kachkaev May 11, 2018

kachkaev May 11, 2018 •

edited

Loading

lfades May 17, 2018

lfades May 17, 2018

kachkaev May 17, 2018

kachkaev May 17, 2018 •

edited

Loading

lfades commented May 18, 2018

kachkaev commented Jun 7, 2018

stale bot commented Nov 23, 2018

stale bot commented Nov 30, 2018

The deduplicator now accepts a query param #312

The deduplicator now accepts a query param #312

Conversation

lfades commented May 11, 2018

Choose a reason for hiding this comment

kachkaev May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kachkaev May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kachkaev May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kachkaev May 17, 2018 • edited Loading

Choose a reason for hiding this comment

lfades commented May 18, 2018

kachkaev commented Jun 7, 2018

stale bot commented Nov 23, 2018

stale bot commented Nov 30, 2018

kachkaev May 11, 2018 •

edited

Loading

kachkaev May 11, 2018 •

edited

Loading

kachkaev May 11, 2018 •

edited

Loading

kachkaev May 17, 2018 •

edited

Loading