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

addResolversToSchema uses schema transforms that do not modify original schema #19

Closed
yaacovCR opened this issue Aug 27, 2019 · 1 comment

Comments

@yaacovCR
Copy link
Owner

yaacovCR commented Aug 27, 2019

In general, graphql-tools uses two methods to generate modified/new schemas:

  1. Direct modification of existing schema: eg addResolversToSchema, addMockFunctionsToSchema, and the SchemaVisitor customizable approach.

  2. Schema recreation: eg mergeSchemas, transformSchema base method, visitSchema and the individual schema transforms.

The difference is important. Although addResolversToSchemas returns the modified schema, some callers might rely on the fact that the reference to the original schema should also reflect the new resolvers.

This is fine as long as API is clear, but two problems are noted:

A: addResolversToSchemas can also be used to add definitions for enums/scalars, but then it also requires reparsing of default values. As currently implemented, this uses transforms that rely on visitSchema and do not modify the original schema. Callers that are using the returned value of addResolversToSchemas are fine, but if they are using the original reference, they would not get the new default values.

B: overall, visitSchema seems to be deprecated in favor of SchemaVisitor. The individual transforms can modify the passed schema or not, because the
true underlying original schema has already been wrapped at that point, so it would be ok to switch to SchemaVisitor or direct modification. It is ok to keep it this way, however. transformSchema uses a version of visitSchema that must recreate the schema and then updates resolvers. This could easily be refactored to recreate the schema with toConfig and then just use addResolversToSchemas to replace resolvers. This is also not strictly necessary, but if visitSchema can be completely deprecated, this action item might be a good long term goal.

@yaacovCR
Copy link
Owner Author

yaacovCR commented Sep 1, 2019

Will create new issue for B.

yaacovCR added a commit that referenced this issue Sep 22, 2019
yaacovCR added a commit that referenced this issue Sep 22, 2019
yaacovCR added a commit that referenced this issue Sep 22, 2019
yaacovCR added a commit that referenced this issue Sep 22, 2019
yaacovCR added a commit that referenced this issue Oct 3, 2019
yaacovCR added a commit that referenced this issue Oct 25, 2019
yaacovCR added a commit that referenced this issue Oct 25, 2019
yaacovCR added a commit that referenced this issue Nov 4, 2019
yaacovCR added a commit that referenced this issue Dec 31, 2019
yaacovCR added a commit that referenced this issue Dec 31, 2019
yaacovCR added a commit that referenced this issue Jan 8, 2020
yaacovCR added a commit that referenced this issue Jan 21, 2020
yaacovCR added a commit that referenced this issue Feb 27, 2020
yaacovCR added a commit that referenced this issue Mar 26, 2020
yaacovCR added a commit that referenced this issue Mar 26, 2020
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

1 participant