graphql-tools icon indicating copy to clipboard operation
graphql-tools copied to clipboard

Certain transforms cannot be used with `delegateToSchema` because MapFields does not initialize transformers in its ctor

Open ntziolis opened this issue 4 years ago • 9 comments
trafficstars

As of now when trying to use certain transformers as part of delegateToSchema they will fail as the underlying MapFields requires transformSchema to be called (and delegateToSchema only calls the operational parts of the transform). Namely we were trying to leverage HoistField when extending schema as part of a graphql mesh setup.

To Reproduce

  • Setup a graphql schema + resolvers
  • Use delegateToSchemain one of the resolvers
  • Use HoistField as a transform in the delegateToSchema call
  • Execution will fail as transformSchemahas not been called

Looking at the code, the only reason transformers is not initialized as part of the constructor is the need to know the name of the Subscription type: https://github.com/ardatan/graphql-tools/blob/c42e811d8562f860d25d6698d84f236c595deae1/packages/wrap/src/transforms/MapFields.ts#L68

Key Question: Is there any way the above can be avoided or structured differently so we can use transforms that rely on MapFields when using delegateToSchema?

Expected behavior

  • Transforms should initialize correctly independent from calling transformSchema
  • Since MapFields is used in many of them it would make sense to remove the dependency on transformSchema being called first.
  • This way they can be used as transforms in delegateToSchema calls

Environment:

Additional context As of right now we are creating an instance of hoist field and call transformSchema manually before passing the instance into the delegateToSchema transforms array. It does work, but is obviously not the original intend. In addition it hurts performance as I happens each time the resolver is called.

ntziolis avatar Sep 22 '21 04:09 ntziolis

Why not just call transformSchema ?

you can do a bit more ergonomically by delegating to a Subschema object rather than just a subschemaConfig object.

a subschema object takes a subschemaConfig and runs transformSchema in its constructor.

you should take care to create the subschema object at startup rather than at runtime, because unless you have very complex dynamic transforms set up, it’s much more efficient to call transformSchema only once.

see example in tests: https://github.com/ardatan/graphql-tools/blob/418e4653398452086d5a1e55ce7255845fbff9a6/packages/delegate/tests/transforms.test.ts#L47

yaacovCR avatar Sep 23 '21 10:09 yaacovCR

In terms of why subscription type name cannot be assessed via the typeName field, see https://github.com/graphql/graphql-spec/pull/776

yaacovCR avatar Sep 23 '21 10:09 yaacovCR

@yaacovCR Thank you for the timely reply. The issue is that we are doing this as part of graphql-mesh inside a composite resolver. We can absolutely call transformSchema but wit would have to be inside the resolver which means at every request.

That said, I see that this is an issue that is rooted in the graphql spec. Totally understand that this requires the workaround in place currently.

Do you know of any way to initialize a sub schema during startup and storing it somewhere where it is accessible from composite resolvers when using graphql-mesh?

Alternatively we will build a mesh hoist transform and submit a PR to the mesh repo.

ntziolis avatar Sep 23 '21 10:09 ntziolis

Sounds like more of an issue on the mesh side, I guess. @ardatan probably has ideas. You could try memoizing maybe the call to transform Schema so it only gets called once?

yaacovCR avatar Sep 23 '21 10:09 yaacovCR

@yaacovCR I understand that at first glance this might look like a mesh issue, but bare with me:

  • We are using mesh to wrap a rest api in graphql and create relations via extending the schema that is automatically generated
  • For this mesh has allows extending the schema (graphql notation) + resolvers via configuration
    • Schema extension is provided via graphql notation
    • Resolvers are provided (in our case) as a path to a js file that exports an object that contains resolvers
  • Mesh then adds this schema + resolvers during startup
  • We have no control of the startup process
  • One of the resolvers we have provided via the above configuration uses a delegateToSchema call that requires the HoistField transform.
  • But when providing transforms directly to the delegateToSchema call the transformSchema method on the transform is not called by design, which totally makes sense (only the operation transforms are called)

Our goal is:

  • preserve all original downstream api calls and data structure
  • only add relations where we need them

So that leaves us with 3 options:

  • Call transformSchema Inside the resolver
    • We do this by creating an instance of the HoistField transform instance and manually call transformSchema before passing it to the delegateToSchema call
  • Create a mesh transform for HoistField
    • This however would actually change the schema, hence we loose some functions of the downstream api
  • Abandon graphql-mesh and replicate all the goodness directly with graphql-tools (this would be the last resort on our end since we rely on lots of mesh goodness)

Summary and why we opened this issue in graphql-tools:

  • A HoistField transform instance cannot be passed into the inline transforms of a delegateToSchema without manually calling transformSchema prior -The only reason based on code review is that transformSchema inside of MapFields needs to be called to ensure the resolvers array is properly initialized
  • This likely applies to other transforms as well that leverage MapFields
  • Hence when removing this requirement more transforms can be used as inline transforms of a delegateToSchema call

That said:

  • We understand now that removing this requirement is not going to happen any time soon due to the graphql-spec issue you have linked
  • We are open for any additional options if any to workaround this in the meantime
    • We are already trying your memoizing approach already and will report here if this gets us the result we are looking for

ntziolis avatar Sep 24 '21 05:09 ntziolis

I hear you, but I don’t have a great workaround that preserves subscription support. Maybe you could try memoizing a call to transform Schema either directly or by using the Subschema class approach outlined above.

yaacovCR avatar Sep 26 '21 02:09 yaacovCR

Or submitting a mesh issue to allow some general startup initialization for later use in custom resolvers.

yaacovCR avatar Sep 26 '21 02:09 yaacovCR

I believe the issue over here can probably be closed, as the solution you require is unlikely to be provided as part of graphql-tools.

Maybe the conversation can continue over graphql-mesh. In the meantime, I hope the newly released @graphql-mesh/transform-replace-field can be useful for your use-case.

santino avatar Oct 06 '21 11:10 santino

@santino I agree there is nothing to be done at the moment. So no need to keep this issue open. That lets keep this in mind wehn the spec changes as the general goal should still be to enable as many transforms as possible to be compatible "out of the box" with delegateToSchema. By that I mean where not absolutely required a call to transformSchema should be optional.

I don't think this is a mesh issue as such either. We simply have to wait if the spec evolves in a way that this can be done.

ntziolis avatar Oct 10 '21 09:10 ntziolis