Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(gatsby-source-graphql): Add transformSchema option #25048

Merged
merged 10 commits into from
Aug 6, 2020

Conversation

ariadne-github
Copy link
Contributor

Description

Should resolve #23552. Adds an optional custom transformSchema option to gatsby-source-graphql that passes the default transforms, schema and link. I has many potential usages, but the consumer should be able to use the same default transforms or tweak or replace them completely. The original use case is to add an argument to the generated "fieldName" field that can then be read by a custom apollo-link to customize the underlying http call with a particular header. Tagging @vladar. This is a proposed implementation that should be safe when not used, and works for my use case. I'm not sure if other parameters might be passed along the transformSchema arguments, maybe plugin options? Maybe the graphql-tools default transformSchema?

Documentation

Example usage that replaces the third default transform and the default link.

// gatsby-config
const { transformSchema } = require(`graphql-tools`);

function customTransformSchema({ schema, link, resolver, defaultTransforms }) {
	const transforms = [defaultTransforms[0], defaultTransforms[1]];
	return transformSchema(
		{
			schema,
			link: myCustomLink(link),
		 },
		[
			defaultTransforms[0], 
			defaultTransforms[1], 
			new MyCustomNamespaceUnderFieldTransform(resolver)
		]
    )
};

{
	...
	plugins: [
		...
		{
		  resolve: "gatsby-source-graphql",
		  options: {
			typeName: "myTypeName",
			fieldName: "myField",
			url: "myEndpoint",
			transformSchema: customTransformSchema
		  },
		},
		...
	],
	...
}

@gatsbyjs/learning.

Related Issues

Resolves #23552.

Should resolve gatsbyjs#23552. Adds an optional custom transfomSchema option to gatsby-source-graphql that passes the default transforms, schema and link. I has many potential usages, but the consumer should be able to use the same default transforms or tweak or replace them completely. The original use case is to add an argument to the generated "fieldName" field that can then be read by a custom apollo-link to customize the underlying http call with a particular header. Tagging @vladar. This is a proposed implementation that should be safe when not used. I'm not sure if other parameters might be passed along the transformSchema arguments, maybe plugin options?
@ariadne-github ariadne-github requested a review from a team as a code owner June 17, 2020 10:26
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 17, 2020
@lannonbr lannonbr changed the title [gatsby-source-graphql] add transformSchema option feat(gatsby-source-graphql): Add transformSchema option Jun 17, 2020
@vladar vladar self-assigned this Jun 17, 2020
@vladar vladar added status: inkteam assigned status: needs core review Currently awaiting review from Core team member and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jun 17, 2020
Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ariadne-github Can you please merge master / resolve conflicts? We just upgraded to graphql-tools@6.0 (#24158) so it may require to update the implementation as well.

@yaacovCR
Copy link
Contributor

Anything helpful we can do from the graphql-tools end? Note that graphql-tools WrapFields/WrapType in general might be useful...

Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have an example in How to Use section in README similar to other options?

@ariadne-github ariadne-github requested a review from a team as a code owner July 21, 2020 09:01
@ariadne-github
Copy link
Contributor Author

@vladar I merged again the most recent commits from master and added a section in the README.md. The code is taken from a working example. I have only my use case, which is a bit convoluted. Does anyone see a better approach?

@vladar vladar added the status: needs docs review Pull request related to documentation waiting for review label Jul 21, 2020
Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding docs 🥇

I don't think we need an example. We can just show the default implementation that uses wrapSchema and add a link to graphql-tools docs on schema wrapping: https://www.graphql-tools.com/docs/schema-wrapping

- resolver (default resolver)
- defaultTransforms (an array with the default transforms)
- options (plugin options)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to add a note that the return value is expected to be the final schema used for stitching.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, basically, I should only leave this as a reference?

// transform-schema.js
const { wrapSchema } = require(`@graphql-tools/wrap`)
const { linkToExecutor } = require(`@graphql-tools/links`)

const transformSchema = ({
  schema,
  link,
  resolver,
  defaultTransforms,
  options,
}) => {
  return wrapSchema(
    {
      schema,
      executor: linkToExecutor(link),
    },
    defaultTransforms
  )
}

module.exports = transformSchema

I just think that would be a bit vague for anyone else to do something useful without knowing at least how the NamespaceUnderFieldTransform is implemented. But if it's good for you, it's good for me too.

What if I link to this issue somewhere to leave a hint?

Copy link
Contributor

@vladar vladar Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this feature in general (as I see it) is that it requires a deep understanding of plugin internals and graphql-tools anyways.

I doubt that the example will be enough for anyone to actually use it. People will probably have to dig into the plugin source code anyways. But it may be rather confusing to other people who don't use this feature.

So we need to surface that this feature exists in docs and leave hints on what other knowledge people may need to actually use it.

Adding a link to the original issue makes sense to me though. I think it's ok to go ahead and do this change. Then let's wait for the learning team review.

Copy link
Contributor

@yaacovCR yaacovCR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but I think the term transformSchema is somewhat overloaded here, because each individual transform also includes a transformSchema method.

What this option is trying to do, I believe, is generate wrapSchema arguments, given some of the other plugin arguments and the default transforms, maybe it could be called customWrapSchemaOptionsFn and instead of calling wrapSchema, just generate arguments that plugin passes through, that way it can mesh well with graphql-tools docs...

@yaacovCR
Copy link
Contributor

To update my comment above, you could keep as is, just rename to customWrapSchemaFn, and then graphql-tools wrapSchema function could be replaced entirely with a custom implementation.

My unsolicited two cents is that transformSchema a bit too generic...

@ariadne-github
Copy link
Contributor Author

To update my comment above, you could keep as is, just rename to customWrapSchemaFn, and then graphql-tools wrapSchema function could be replaced entirely with a custom implementation.

My unsolicited two cents is that transformSchema a bit too generic...

I've updated the code as requested by @yaacovCR, and I also updated and simplified the documentation.

Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being late to the party again 😅

I don't think customWrapSchemaFn is a good name for this option. We have other options in this plugin: createSchema, createLink, fetch.

customWrapSchemaFn sounds pretty inconsistent with those. We don't use customCreateSchema or customFetch. Also, all of them are kind of ambiguous as they override existing functionality.

So I think we should either use wrapSchema for the option name or return to the transformSchema variant.

@yaacovCR what would be the least evil in your opinion? Or maybe you have other thoughts on this?

@yaacovCR
Copy link
Contributor

createWrappedSchema?

@vladar
Copy link
Contributor

vladar commented Jul 24, 2020

I think createWrappedSchema is easy to confuse with createSchema. I actually think we should revert back to transformSchema.

I understand that for someone closely familiar with graphql-tools this sounds ambiguous but for the plugin user it clearly communicates the intent of this option. If you are ready to dive into graphql-tools level then I don't think this ambiguity will confuse you.

@yaacovCR
Copy link
Contributor

Hi!

I agree with you, but for a different reason. From gatsby perspective, calling wrapSchema is not the only valid way of transforming the original schema, so transformSchema is better.

@yaacovCR
Copy link
Contributor

Sorry @ariadne-github for throwing things off track!

@ariadne-github
Copy link
Contributor Author

In conclusion, do you agree on transformSchema?

@yaacovCR
Copy link
Contributor

Yes. In general, I have found myself to essentially always agree with @vladar :)

Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thanks for the suggestion and the PR!

@vladar vladar added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Aug 6, 2020
@gatsbybot gatsbybot merged commit b61e484 into gatsbyjs:master Aug 6, 2020
@gatsbot
Copy link

gatsbot bot commented Aug 6, 2020

Holy buckets, @ariadne-github — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@ariadne-github ariadne-github deleted the patch-4 branch August 6, 2020 08:12
blainekasten pushed a commit that referenced this pull request Aug 6, 2020
* [gatsby-source-graphql] add transfomSchema option

Should resolve #23552. Adds an optional custom transfomSchema option to gatsby-source-graphql that passes the default transforms, schema and link. I has many potential usages, but the consumer should be able to use the same default transforms or tweak or replace them completely. The original use case is to add an argument to the generated "fieldName" field that can then be read by a custom apollo-link to customize the underlying http call with a particular header. Tagging @vladar. This is a proposed implementation that should be safe when not used. I'm not sure if other parameters might be passed along the transformSchema arguments, maybe plugin options?

* Fix implementation, renamed option

* Added options parameter and readme example

* Removed example + default implementation + references

* Renamed transformSchema in customWrapSchemaFn

* Renamed back option into "transformSchema"

Co-authored-by: Stefano De Pace <depace@ARIADNEAD.DOM>
Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>
johno pushed a commit that referenced this pull request Aug 6, 2020
* [gatsby-source-graphql] add transfomSchema option

Should resolve #23552. Adds an optional custom transfomSchema option to gatsby-source-graphql that passes the default transforms, schema and link. I has many potential usages, but the consumer should be able to use the same default transforms or tweak or replace them completely. The original use case is to add an argument to the generated "fieldName" field that can then be read by a custom apollo-link to customize the underlying http call with a particular header. Tagging @vladar. This is a proposed implementation that should be safe when not used. I'm not sure if other parameters might be passed along the transformSchema arguments, maybe plugin options?

* Fix implementation, renamed option

* Added options parameter and readme example

* Removed example + default implementation + references

* Renamed transformSchema in customWrapSchemaFn

* Renamed back option into "transformSchema"

Co-authored-by: Stefano De Pace <depace@ARIADNEAD.DOM>
Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes status: needs core review Currently awaiting review from Core team member status: needs docs review Pull request related to documentation waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gatsby-source-graphql-multiple - fork, feature or estension?
4 participants