Expose sourceHash or id for operations

[artola]

fetchQueryreceives operation.node.params but it would be nice to have access to operation.node.hash to use it for local cache key and mainly for the following approach: first fetch to the server with hash and variables only, if the server contains the hash in its own cache, it directly responds, else a second round trip with query, variables and hash will be done.

relay/packages/relay-runtime/store/RelayModernEnvironment.js

Line 274 in b783255

operation.node.params,

relay/packages/relay-compiler/language/javascript/formatGeneratedModule.js

Line 45 in 4343e80

(node/*: any*/).hash = '${sourceHash}';

Other possibility could be using something similar to "persist-output", but this removes the text of the operation, we need to keep the text and get also the id to use this one as hash.

@josephsavona Do you have some hint? is this doable? willing to work in a PR if it makes sense for you.

[kassens]

The hash is just the hash of the source directly not taking into account the referenced fragments to allow us to warn in the babel plugin in dev mode if you forgot to run the Relay compiler and the artifact is outdated. So hash shouldn't be used outside of Relay as it doesn't change if only a fragment is changed.

What we do at facebook is to have the persist function actually send a request to the server to add the operations to a database there.

[artola]

The hash is just the hash of the source directly not taking into account the referenced fragments to allow us to warn in the babel plugin in dev mode if you forgot to run the Relay compiler and the artifact is outdated. So hash shouldn't be used outside of Relay as it doesn't change if only a fragment is changed.

What we do at facebook is to have the persist function actually send a request to the server to add the operations to a database there.

Implement persist solution as FB is beyond our possibilities (integration, etc), while a tweak in the front-end might be doable.

Please consider the possibility for small change in writeRelayGeneratedFile the code block in L108 is only executed for persistOutput.

relay/packages/relay-compiler/codegen/writeRelayGeneratedFile.js

Line 108 in 03d709b

if (persistQuery) {

I do believe that might be possible use the logic in this block with small tweak to expose it in the params later passed to the network.execute:

           params: {
              operationKind: generatedNode.params.operationKind,
              name: generatedNode.params.name,
              id: await persistQuery(text),                              // <=== to add id to the node
              text: null,                                                             // <=== remove to keep value
              metadata: generatedNode.params.metadata,
            },

relay/packages/relay-compiler/codegen/writeRelayGeneratedFile.js

Line 122 in 03d709b

id: await persistQuery(text),

I think that adding this id to the params is just an enhancement and it will not hurt, giving us the possibility to define in the network (fetchQuery) and decide whether send id, text or both ... furthermore the id will be useful for the cache (cache.get(queryID, variables)) because nowadays we use queryID = operation.text and later a shorter queryID = operation.id.

[alloy]

I’m planning on using a persist function as per @kassens’ description in the future, so for now we simply fetch the query text from the persisted query map https://github.com/artsy/emission/blob/758258699f78db09513b61a321c6099a5c15ee07/src/lib/relay/middlewares/cacheMiddleware.ts#L25

[artola]

I’m planning on using a persist function as per @kassens’ description in the future, so for now we simply fetch the query text from the persisted query map https://github.com/artsy/emission/blob/758258699f78db09513b61a321c6099a5c15ee07/src/lib/relay/middlewares/cacheMiddleware.ts#L25

The problem with this approach is that "all" the queries get bundled into the app, even that removed by tree-shacking. I might monkey patch the relay-compiler with 2 lines of code to:

Nowadays if we "persist" then we get just the id, else we get just text. Because of:

relay/packages/relay-compiler/codegen/writeRelayGeneratedFile.js

Line 121 in 582c706

text: null,

I would like to have both. Why? We manage different strategies to deal with server persistence, but for simplicity imagine that the GQL client send id and text, if id is in the cache of the server it does not need to parse it, else could use the text and store it.

cc @josephsavona How do you see to introduce a flag to choose the id and/or text inclusion?

[artola]

@jstejada Do you think that explained above might be possible expose both id and text?

I will appreciate any hint, thanks.

[eMerzh]

Hey,
i sumble upon this, i'm aslo in the process of implementing "persisted queries" in form of "cached via ID" so i'll be interested too to have both ID and text in the generated files.
we've also had to create a custom "relay-compiler" to handle this...

it there something preventing adding ID every-time ? (and keep the behavior of nulling the text with the persisted option)

[artola]

Hey,
i sumble upon this, i'm aslo in the process of implementing "persisted queries" in form of "cached via ID" so i'll be interested too to have both ID and text in the generated files.
we've also had to create a custom "relay-compiler" to handle this...

it there something preventing adding ID every-time ? (and keep the behavior of nulling the text with the persisted option)

Our flow is: send to the server id and text, the server will check in the cache for that id, else will verify (to avoid some hack) that the text match the id (md5) and store it.
With this small improvement, our server does not need to parse and validate again and again the same query. It is a big performance enhancement.

In this moment the compiler logic is: if it is a persisted query, then just give back the id (text is nullified), if it is not persisted give back text (id is null).

The required change is just add id for non-persisted queries. I do not see any side-effect on just adding this hash by default, it is only 32 chars in a magnitude of thousands.

@josephsavona @kassens Could you please evaluate the feasibility of this change?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[eMerzh]

Is there anything new with about this with the relay-compiler beeing moved to rust?
like if it was a "speed" issue to do both aproach (id + text) should now be ok?

Thanks 🙏

[eMerzh]

I think it's now the case , we can access the operation.id... unfortunately we can't change the algorithm yet without using persisting queries file