facebook / relay

Relay is a JavaScript framework for building data-driven React applications.
https://relay.dev
MIT License
18.41k stars 1.83k forks source link

The same fragment yields a different hash value...? #4226

Open kmin-283 opened 1 year ago

kmin-283 commented 1 year ago

fragment

fragment SellingProductListFragmentContainer_store_representActiveProducts on Store
      @argumentDefinitions(
        count: { type: "Int", defaultValue: 10 }
        cursor: { type: "ID" }
        filter: { type: "ProductFilter", defaultValue: { statuses: [ACTIVE], representStatus: ACTIVE } }
      )
      @refetchable(queryName: "SellingProductListFragmentContatinerRepresentActiveProducts") {
        representActiveProducts: products(first: $count, after: $cursor, filter: $filter)
          @connection(key: "SellingProductListFragmentContainer_store_representActiveProducts") {
          edges {
            node {
              _id
            }
          }
        }
      }

When the relay-compiler command is executed, generated the hash value is

Screenshot 2023-02-20 at 4 44 26 PM

This result is presumably due to this code working. https://github.com/facebook/relay/blob/c0cc17a07e1f0c01f3e5c564eed50b5a30f4228f/compiler/crates/relay-compiler/src/build_project/build_ir.rs

but when i execute same fragment in this code it return different hash value

const { parse, print } = require('graphql')
const crypto = require('node:crypto')

const gql = String.raw
const text = gql`
  fragment SellingProductListFragmentContainer_store_representActiveProducts on Store
  @argumentDefinitions(
    count: { type: "Int", defaultValue: 10 }
    cursor: { type: "ID" }
    filter: { type: "ProductFilter", defaultValue: { statuses: [ACTIVE], representStatus: ACTIVE } }
  )
  @refetchable(queryName: "SellingProductListFragmentContatinerRepresentActiveProducts") {
    representActiveProducts: products(first: $count, after: $cursor, filter: $filter)
      @connection(key: "SellingProductListFragmentContainer_store_representActiveProducts") {
      edges {
        node {
          _id
        }
      }
    }
  }
`

const ast = parse(text)
const definition = ast.definitions[0]
const printed = print(definition)
const hash = crypto.createHash('md5').update(printed, 'utf8').digest('hex')

console.log(hash)

the hash value is

296c46ae6772194c8404c631edebccbf

I put the same input into the md5 hash function, but why are the results different?

cometkim commented 6 months ago

Hey @kmin-283, I recently received a report of the same issue from https://github.com/cometkim/vite-plugin-relay-lite/issues/53

And found https://github.com/graphql/graphql-js/pull/2797 from graphql-js' v15.4.0 release.

The problem is, that because there is a discrepancy in the printer implementation of graphql-js and relay-compiler (and any other), they cannot be considered the same (even though both came from Meta).

The printer is not part of the GraphQL spec. However, as GraphQL is a platform-agnostic technology, we would continue to experience similar problems without formal standardization.

cometkim commented 6 months ago

Anyway, at least relay-compiler is assumed to interoperate with graphql-js, so this should be fixed in here IMO

captbaritone commented 6 months ago

If I understand correctly, fixing it here in the Relay compiler will also require all users to ensure they upgrade the graphql-js version imported by their Babel plugin? I believe SWC also has an implementation of our transform. I wonder how they do hash computation and if a change to how we compute them in Relay would require a breaking change to that implementation as well?

eMerzh commented 6 months ago

seeing from this https://github.com/swc-project/plugins/blob/main/packages/relay/transform/src/lib.rs i don't this swc is doing query transformation or hashing only "query to file".

about babel, mmh i guess yes 🤔 ...

cometkim commented 6 months ago

fixing it here in the Relay compiler will also require all users to ensure they upgrade the graphql-js version imported by their Babel plugin?

Umm.. yes? Or need to downgrade to <=v15.3.0...

And it's not just Babel's dependency. This could be if the users have another GraphQL tooling in their project or a monorepo with the graphql-js server.

To handle it correctly, specify "graphql-js <= 15.3.0" as optional peerDeps in the patch version and change it to ^15.4.0 in the fix version.

cometkim commented 6 months ago

I had some chats with @benjie about this in the GraphQL community discord

It seems to boil down to a few viable options:

  1. Fix the current Relay compiler's printer to match with the graphql-js's behavior, and specify peer dependency version range strictly
  2. Make the Relay compiler's printer as "canonical printer" of the Relay ecosystem. And ensure the babel plugin, vite plugin, etc use that instead of the graphql-js' printer
  3. Standardize a new spec for stable runtime representation of trusted documents. (see discussion for the detail)

It seems 1 is the easiest option to choose here, but 2 is more "correct" moving.

btw I have some crazy ideas for option 3, which I will introduce later.

captbaritone commented 6 months ago

I like 2, but I wonder about the performance implications. Would the Babel plugin need to shell out to the Relay compiler for each document?

A variant of 1 would be to make the Relay compiler configurable and have it be able to emit either format.

cometkim commented 6 months ago

Would the Babel plugin need to shell out to the Relay compiler for each document?

As the maintainer of vite-plugin-relay-lite, I would write/port it to JS implementation.. :innocent: others may use WASM module of the parser and printer I guess

cometkim commented 6 months ago

@captbaritone a quick check, can the current Relay compiler's printer implementation be considered "stable", or would it be fixed soon or later to match the graphql-js'? Once I port the printer, it's also possible to contribute to the babel plugin as well.

benjie commented 6 months ago

(Not being at all familiar with Relay's internals...) It feels like you should break Relay's printer into it's own tiny module so it can easily be added as a dependency to other tooling? Then this printer could follow semver, any formatting change would require a semver major release.

cometkim commented 6 months ago

Then this printer could follow semver, any formatting change would require a semver major release.

Sure it does.

I was wondering if it might be included in the next major version plans. If the Relay compiler printer will keep current behavior as-is, this will require a different kind of user guide IMO (especially for Babel users).