mercurius-js / cache

Adds an in-process caching layer to Mercurius. Federation is fully supported.
MIT License
106 stars 19 forks source link

fix: upgrade to new gateway and federation #127

Closed marco-ippolito closed 1 year ago

marco-ippolito commented 1 year ago

Resolves: https://github.com/mercurius-js/cache/issues/126 I've updated to mercurius 12. I've bumped the version to add support for the new packages mercurius-federation and mercurius-gateway. I've put this line under coverage ignore:

  // istanbul ignore next
  if (app.graphqlGateway) {
    // Add hook to regenerate the resolvers when the schema is refreshed
    app.graphql.addHook('onGatewayReplaceSchema', async (instance, schema) => {
      buildCache()
      setupSchema(schema, policy, all, cache, skip, onDedupe, onHit, onMiss, onSkip, onError, report)
    })
  }

since I couldnt come up with a meaningful test for onGatewayReplaceSchema (the one on gateway is very flaky), open to suggestion

simone-sanfratello commented 1 year ago

Happy to work on a solution for test, not going to accept code without

mcollina commented 1 year ago

The ignore should not be needed, because that if should be covered by the gateway test. If it's not then there is a bug.

marco-ippolito commented 1 year ago

it is not covered and me and @simone-sanfratello tried some basic test replacing schema on the gateway but it doesnt improve coverage. I'm trying to come up with a test thats non flaky

simone-sanfratello commented 1 year ago

From our check, looks like onGatewayReplaceSchema hook is not been triggered

mcollina commented 1 year ago

It might be a problem in mercurius gateway then, or if all tests are passing without that... maybe it's not needed anymore?

simone-sanfratello commented 1 year ago

That's what we suppose too. @marco-ippolito is going to investigate on the mercurius gateway first, then we'll come back here

simone-sanfratello commented 1 year ago

I think we should update the cache rules on schema change, for example using all option I expect all the queries to be updated too

marco-ippolito commented 1 year ago

~~I've added a test that shows that gatewatOnReplaceSchema it's not triggered by replace schema on a service. @mcollina might be a bug from the gateway. What it should do is reset the cache since the schema changed but its not triggered.~~

mcollina commented 1 year ago

Is this still wip? @marco-ippolito could you update the PR title if it's ready to go?

I think this is good to land.

@simone-sanfratello PTAL.

marco-ippolito commented 1 year ago

Its ready, it has full test coverage