Skip to content
This repository has been archived by the owner on Aug 20, 2020. It is now read-only.

Align persistgraphql output file format with proposed apollo-codegen operation ID mapping file #34

Open
kompfner opened this issue Jun 28, 2017 · 2 comments

Comments

@kompfner
Copy link

In a recent PR to apollo-codegen, a mechanism for generating unique operation IDs was introduced. In addition, it introduced a generated output file that maps between these IDs and the corresponding operation text, for use in registering operations with a server for persistence and whitelisting. The rationale for not simply using persistgraphql is contained in the PR description.

This proposed output file differs from persistgraphql's output in a few important ways:

  1. It uses operation hashes as the IDs, rather than an incrementing counter.
  2. It uses the ID as the mapping’s key, rather than its value.
  3. It maps to a JSON object that itself contains the operation source (text) and its name.

As pointed out by @martijnwalraven, there are good reasons to align the approaches taken by apollo-codegen and persistgraphql:

One area in particular where I think standardization would be helpful is in the generated mapping file. Ideally, these mapping files would be supported by different servers and tools in the same way the extracted_queries.json file that persistgraphql generates is currently supported by Scaphold for example.

So I'm not saying we should stick to what persistgraphql currently does, but I'd at least like us to synchronize the changes (like switching to hashes or changing the mapping format).

There is an existing issue on persistgraphql that addresses difference point 1. I think switching to operation hashes is the right approach.

What I’d like to propose here is that, to address difference points 2 and 3, persistgraphql adopt the approach proposed in the apollo-codegen PR. The PR lays out an argument for this approach, which I’ll recap here:

  • It seems like (for the server at least) the mapping from ID —> operation text is more useful than the reverse, suggesting that ID should be the key.
  • Typically, keys are short unique identifiers, and they map to values that contain more information, not less, again suggesting that ID should be the key.
  • We already have two pieces of information in each value (operation name and text), which our servers are prepared to consume at registration time. You can imagine other fields coming in the future: perhaps comments describing the operations, for display in a dashboard listing all persisted operations. This suggests that the value in the mapping should be a JSON object that can be extended in the future without breaking parsing older fields.
@martijnwalraven
Copy link

I think these changes make a lot of sense. @Poincare, what do you think?

We may want to add a root object and put the operations under an operations key. I think it would be great if we could specify some standard metadata, like app name and version, which a server or other tool can use to tag operations.

@Poincare
Copy link
Contributor

@martijnwalraven @kompfner This makes sense. I haven't yet delved deeply into the apollo-codegen PR but the core idea of using the operation hashes seems great. Will look into this in more detail shortly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants