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

[WIP] [schema] Allow registering custom field extensions #13738

Merged
merged 9 commits into from May 14, 2019

Conversation

@stefanprobst
Copy link
Contributor

commented Apr 30, 2019

This is a WIP for allowing custom GraphQL field extensions. This is how it works:

The registerFieldExtension API accepts an extension consisting of a description, extension options, and a process function that should return a (partial) field config which will be merged into the existing field config. The extension can decide to wrap or overwrite an existing resolver.

Example:

exports.registerFieldExtension = ({ registerFieldExtension }) => {
  registerFieldExtension(`volume`, {
    description: `Adjust the volume.`,
    args: {
      loud: { type: `Boolean` },
    },
    // Return a new partial field config to extend the previous field config with.
    // Receives extension `args` and the current field config as parameters.
    process(defaults, prevFieldConfig) {
      return {
        type: `String`,
        args: {
          loud: { type: `Boolean` },
        },
        resolve(source, args, context, info) {
          const fieldValue = source[info.fieldName]
          const shouldUpperCase = args.loud != null ? args.loud : defaults.loud
          return shouldUpperCase ? fieldValue.toUpperCase() : fieldValue
        },
      }
    },
  })
}

It can then be used either with Gatsby Type Builders, or in SDL, e.g.:

exports.sourceNodes = ({ actions }) => {
  actions.createTypes(`
    type Site implements Node {
      siteMetadata: SiteMetadata
    }
    type SiteMetadata {
      title: String @volume(loud: true)
    }
  `)
}

Would love to get feedback on this!

Continued from #13623

@stefanprobst stefanprobst requested a review from gatsbyjs/core as a code owner Apr 30, 2019

@freiksenet

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

The code example doesn't feel right. I think resolver should get normal arguments of the field and args should come from the process arguments. Is there a typo there?

@freiksenet

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

So what it should be like is more

  process(extensionArgs, prevFieldConfig) {
    return {
        type: `String`,
        args: {
          loud: { type: `Boolean` },
        },
        resolve(source, args, context, info) {
          const fieldValue = source[info.fieldName]
          const shouldUpperCase = extensionArgs.loud != null ? extensionArgs.loud : defaults.loud
          return shouldUpperCase ? fieldValue.toUpperCase() : fieldValue
        },
      }
    },
@freiksenet

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

It seems to be correct in code, just not in the example.

@freiksenet
Copy link
Contributor

left a comment

This is cool, I'm happy with this API. 👍

Let's get rid of the global state. Also needs tests.

const extensions = typeComposer.getFieldExtensions(fieldName)
Object.keys(extensions)
.filter(name => !internalExtensionNames.includes(name))
.sort(a => a === `proxyFrom`) // Ensure `proxyFrom` is run last

This comment has been minimized.

Copy link
@freiksenet

freiksenet Apr 30, 2019

Contributor

I don't like those special cases. Now it feels extensions should be a list.

This comment has been minimized.

Copy link
@wardpeet

wardpeet May 3, 2019

Member

won't you have the same issue with a list?

}
}

const registerExtensions = ({ parentSpan }) =>

This comment has been minimized.

Copy link
@freiksenet

freiksenet Apr 30, 2019

Contributor

No reason to have separate API. Let's make it an action like createTypes.

This comment has been minimized.

Copy link
@stefanprobst

stefanprobst Apr 30, 2019

Author Contributor

Let's first quickly discuss how happy we are with createTypes being an action? The issue is that actions get passed to all Node APIs but we need it to be called before schema generation, that's why createTypes should really only be called in sourceNodes even though it is available in other APIs as well. But I'm happy if we make this the general convention to follow.

This comment has been minimized.

Copy link
@freiksenet

freiksenet Apr 30, 2019

Contributor

Okay, let's do it this way:

  1. Add deprecation warnings to actions for createTypes and third party schema adding.
  2. Create a schema customization runner API that is called after source nodes. Have createTypes, thirdPartySchemas and this new stuff available there.
  3. Add all further schema customization stuff there.
  4. Update source-graphql to use 3 instead of 1.

This comment has been minimized.

Copy link
@stefanprobst

stefanprobst May 2, 2019

Author Contributor

Sounds good! Probably should be in a separate PR?

This comment has been minimized.

Copy link
@wardpeet

wardpeet May 3, 2019

Member

Separate PR sounds good!

This comment has been minimized.

Copy link
@freiksenet

freiksenet May 3, 2019

Contributor

Yeah, let's merge the thing without customization, then have a separate PR together with Redux changes.

`plugin`,
]

const typeExtensions = {

This comment has been minimized.

Copy link
@freiksenet

freiksenet Apr 30, 2019

Contributor

This global state should be in Redux and passed from the top like for other global state.

This comment has been minimized.

Copy link
@stefanprobst

stefanprobst Apr 30, 2019

Author Contributor

Agreed

description: `Resolver options. Vary based on resolver type.`,
},
},
process(args, fieldConfig) {

This comment has been minimized.

Copy link
@freiksenet

freiksenet Apr 30, 2019

Contributor

👍 Really like this API.


addResolver: {
description: `Add a resolver specified by "type" to field.`,
args: {

This comment has been minimized.

Copy link
@freiksenet

freiksenet Apr 30, 2019

Contributor

We have a bit of a weird discrepancy, where directives do type checking for arguments, but extensions applied directly won't. Not sure if we should solve and if we should then how. One way would be to manually use args to validate.

This comment has been minimized.

Copy link
@stefanprobst

stefanprobst Apr 30, 2019

Author Contributor

Yeah good point! Hmm yes probably need to manually do this for extensions

@stefanprobst

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

The code example doesn't feel right.

Hmm, could you point me to what's wrong? The process function gets the extension args as the first argument (which is called defaults here), and the field resolver gets the field args as the second argument (args here) as usual. So in the resolver we use the field arg if we get one, and fall back to the extension default arg if not.

@freiksenet

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

@stefanprobst nm my comment about example, I misread everything :D It's actually correct.

},
},

dateformat: {

This comment has been minimized.

Copy link
@freiksenet

freiksenet May 2, 2019

Contributor

Not 100% sure we should add them separately.

This comment has been minimized.

Copy link
@stefanprobst

stefanprobst May 2, 2019

Author Contributor

I don't think there are disadvantages to adding the shortcuts as well. The advantage otoh is that it allows a more concise way to add functionality to fields. E.g. adding foreign-key relations (which is probably the most common use-case) can be shortened to

field: SomeType @link

instead of

field: SomeType @addResolver(type: "link")

This comment has been minimized.

Copy link
@freiksenet

freiksenet May 3, 2019

Contributor

I'm now considering not having addResolver, but having those instead.

The original motivation was that you'll have new resolvers to add, but API would be the same with this extension api.

@wardpeet
Copy link
Member

left a comment

Added some comments, nothing big. Like always a bit of bikeshed

Show resolved Hide resolved packages/gatsby/src/utils/api-node-docs.js Outdated
Show resolved Hide resolved packages/gatsby/src/schema/schema.js Outdated
}
}

const registerExtensions = ({ parentSpan }) =>

This comment has been minimized.

Copy link
@wardpeet

wardpeet May 3, 2019

Member

Separate PR sounds good!

Show resolved Hide resolved packages/gatsby/src/schema/extensions/index.js Outdated
const extensions = typeComposer.getFieldExtensions(fieldName)
Object.keys(extensions)
.filter(name => !internalExtensionNames.includes(name))
.sort(a => a === `proxyFrom`) // Ensure `proxyFrom` is run last

This comment has been minimized.

Copy link
@wardpeet

wardpeet May 3, 2019

Member

won't you have the same issue with a list?

.filter(name => !internalExtensionNames.includes(name))
.sort(a => a === `proxyFrom`) // Ensure `proxyFrom` is run last
.forEach(name => {
const { process } = fieldExtensions[name] || {}

This comment has been minimized.

Copy link
@wardpeet

wardpeet May 3, 2019

Member

A little bit of bikeshed 😄

what's the reason for the name process? It doesn't feel completely right to me. Process doesn't really tell me what this function does, does extend make more sense?

some other words that might apply: override, merge, apply

This comment has been minimized.

Copy link
@stefanprobst

stefanprobst May 4, 2019

Author Contributor

No particular reason, I'm open to suggestions -- maybe extend yes

const { getDateResolver } = require(`../types/date`)

// Reserved for internal use
const internalExtensionNames = [

This comment has been minimized.

Copy link
@wardpeet

wardpeet May 3, 2019

Member

should we prefix them? As we won't be able to add any more to the list unless we publish a major as this might break people's code.

This comment has been minimized.

Copy link
@stefanprobst

stefanprobst May 4, 2019

Author Contributor

Good point

wardpeet and others added some commits May 3, 2019

Update packages/gatsby/src/utils/api-node-docs.js
Co-Authored-By: stefanprobst <stefan.probst@univie.ac.at>
Update packages/gatsby/src/schema/schema.js
Co-Authored-By: stefanprobst <stefan.probst@univie.ac.at>
@stefanprobst

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

I've removed the public createFieldExtension API and the addResolver extension. Next steps in follow-up PRs after 2.5 release should be:

  • move schema customization actions to one API Does this need more discussion? I'm 👍
  • move extensions registry into redux
  • add type checking for extension args
  • decide on prefixing internal extension names
  • decide on a name for process function. extend?

@freiksenet freiksenet merged commit 5bf9bb8 into gatsbyjs:infer-fixing May 14, 2019

15 of 16 checks passed

ci/circleci: starters_validate Your tests failed on CircleCI
Details
Danger All good
Details
Peril All green. Good on 'ya.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsbygram Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node6 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details

@stefanprobst stefanprobst deleted the stefanprobst:schema-extensions-next branch Jul 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.