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

transform documents hooks #8723

Merged
merged 18 commits into from
Feb 21, 2023
Merged

Conversation

kazekyo
Copy link
Contributor

@kazekyo kazekyo commented Dec 12, 2022

Description

I have a preset to transform user-defined documents.
I want to use the new client preset, so I want to be able to transform documents with a plugin.

Plugin will have the following functions:

module.exports = {
  plugin: () => {
    return 'hello'
  },
  transformDocuments: (_schema, documents) => {
    // Make some changes to the documents    
    return documents;
  },
  validateBeforeTransformDocuments: () => {
    // Raises an error if necessary
  },
};

Use it as follows:

import type { CodegenConfig } from '@graphql-codegen/cli'
 
const config: CodegenConfig = {
   schema: 'https://localhost:4000/graphql',
   documents: ['src/**/*.tsx'],
   generates: {
      './src/gql/': {
        preset: 'client',
        documentTransformPlugins: ['./my-plugin.js'],
        plugins: ['./my-plugin.js']
      }
   }
}
export default config

One of the main uses of this feature is to add fields.
For example, assuming that there is a @useIcon directive that adds fields related to an icon:

query exampleQuery {
  user @useIcon {
    name
    email
  }
}

If the plugin transforms the document, we can get the following document:

query exampleQuery {
  user {
    name
    email
    iconUrl # Added by plugin
    iconFileName # Added by plugin
  }
}

Related:

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Screenshots/Sandbox (if appropriate/relevant):

Please see unit tests for usage.

How Has This Been Tested?

I added tests to the spec file.

Checklist:

  • I have followed the CONTRIBUTING(https://github.com/the-guild-org/Stack/blob/master/CONTRIBUTING.md) doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Further comments

About validation

No validation of documents is executed before transforming.
Because I understand that validation before transforming often leads to errors when this feature is needed.
This decision will allow future use of plugins such as relay-operation-optimizer without the skipDocumentsValidation flag.

About relay-operation-optimizer

We can probably remove the special implementation for relay-operation-optimizer and replace it with the document transform plugin.
But I did not include that changes in this pull request. If there is no problem, I will do that next.

@changeset-bot
Copy link

changeset-bot bot commented Dec 12, 2022

🦋 Changeset detected

Latest commit: 2a8e14f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@graphql-codegen/cli Minor
@graphql-codegen/core Minor
@graphql-codegen/plugin-helpers Minor
@graphql-codegen/client-preset Minor
@graphql-codegen/gql-tag-operations-preset Minor
@graphql-codegen/graphql-modules-preset Minor
@graphql-cli/codegen Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kazekyo kazekyo marked this pull request as ready for review December 12, 2022 10:34
@kazekyo
Copy link
Contributor Author

kazekyo commented Dec 12, 2022

@saihaj @n1ru4l Could you please review this?
#8593 (reply in thread)

@saihaj saihaj requested review from saihaj and n1ru4l December 12, 2022 22:04
@kazekyo
Copy link
Contributor Author

kazekyo commented Dec 21, 2022

@n1ru4l I have added a page to the documentation. I'm concerned about the quality of my texts as I don't usually use English.
Please feel free to point it out 🙏

@saihaj saihaj requested a review from n1ru4l December 26, 2022 16:57
const { visit, print } = require('graphql')

module.exports = {
plugin(schema, documents, config) {
Copy link
Collaborator

@n1ru4l n1ru4l Feb 3, 2023

Choose a reason for hiding this comment

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

I am not really sure if we need this hook/function. What benefit is it trying to solve?

could this file not simply be the following?

module.exports = function transformDocuments (...args) => {
  
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In terms of the current implementation, I'm using Plugin mechanism to executetransformDocuments. Currently, we get the transformDocuments function from Plugin.

Regarding your question, are you suggesting we create a plugin without the plugin function?
Currently, the plugin function is not optional, but I can change it to be optional. However, I have concerns that making the plugin function optional may confuse users and may not be a desirable change to the API. 🤔

Another option would be not to use the Plugin mechanism at all. Instead, we could introduce a new concept called something like DocumentTransform that works independently of Plugin. Just as Preset is a separate concept from Plugin, DocumentTransform is separate from Plugin.
This would be a slightly over-engineered solution, and most implementations would probably be copies of Plugin, but it would make API cleaner. ✨
If this design is acceptable, I will implement it. 🚀

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the new DocumentTransform concept is definitely what we should do here! Can you please adjust this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@n1ru4l
Copy link
Collaborator

n1ru4l commented Feb 3, 2023

Hey @kazekyo, sorry for letting this PR untouched for so long. I just rebased your PR and I have some questions for you:

With the client-preset we are aiming for a relay-compiler-like experience. Features we want to implement there include the following:

  • fragment masking (done)
  • fragment arguments (pending)
  • operation document optimization (pending)

Our goal would be that all new projects using GraphQL Code Generator would pick the client-preset over other presets/plugin combinations.

This feature would kind of allow implementation fragment arguments and operation document optimization as you already pointed out. This is great as it would work nicely with this change 😍. See this client-preset artifact as an example.

Would you be interested in helping out by having fragment arguments and document optimization built into the client preset? Of course not part of this pull request!


Here is some more feedback that I noticed while playing around with it.

Right now it is impossible only possible to reference documentTransforms via a file.

// eslint-disable-next-line import/no-extraneous-dependencies
import { type CodegenConfig } from '@graphql-codegen/cli';

const config: CodegenConfig = {
  schema: './src/yoga.ts',
  documents: ['src/**/*.ts'],
  generates: {
    './src/gql/': {
      plugins: [],
      preset: 'client-preset',
      documentTransforms: ['./foo.js'],
    },
  },
};

export default config;

Should we also allow inline functions?

// eslint-disable-next-line import/no-extraneous-dependencies
import { type CodegenConfig } from '@graphql-codegen/cli';

const config: CodegenConfig = {
  schema: './src/yoga.ts',
  documents: ['src/**/*.ts'],
  generates: {
    './src/gql/': {
      plugins: [],
      preset: 'client-preset',
      documentTransforms: [{
        tarnsformDocuments() {}
      }],
    },
  },
};

export default config;

Let me know what you think!

@kazekyo
Copy link
Contributor Author

kazekyo commented Feb 4, 2023

@n1ru4l Thanks for reviewing this pull request!


Would you be interested in helping out by having fragment arguments and document optimization built into the client preset? Of course not part of this pull request!

Of course, I'd be happy to help! It seems that some of our goals overlap. 😃
Please guide me to the relevant issues, discussions, Discord threads, etc., to discuss these tasks.

As for this PR, I am unsure whether it can be used as part of your project or should be re-implemented in the client-preset. I'll keep developing it if it can be used, but if you have a different opinion, feel free to close it. 🙏


Should we also allow inline functions?

// eslint-disable-next-line import/no-extraneous-dependencies
import { type CodegenConfig } from '@graphql-codegen/cli';

const config: CodegenConfig = {
  schema: './src/yoga.ts',
  documents: ['src/**/*.ts'],
  generates: {
    './src/gql/': {
      plugins: [],
      preset: 'client-preset',
      documentTransforms: [{
        tarnsformDocuments() {}
      }],
    },
  },
};

export default config;

It's great to allow inline functions. It would make it easier for users to experiment.

Just to confirm, Plugin cannot be written as inline functions, correct?

import { type CodegenConfig } from '@graphql-codegen/cli';

const config: CodegenConfig = {
  schema: './src/yoga.ts',
  documents: ['src/**/*.ts'],
  generates: {
    './src/gql/': {
      plugins: [{
        plugin() {}
      }],
      preset: 'client-preset',
    },
  },
};

export default config;

Currently, I use Plugin to execute transformDocuments, so it would be necessary to allow Plugin to write functions inline. I think a separate pull request for this would be appropriate. 🤔
If we don't use Plugin for transformDocuments, I will implement it in this pull request! 💪

@kazekyo kazekyo removed the request for review from saihaj February 12, 2023 08:13
@kazekyo kazekyo requested a review from n1ru4l February 12, 2023 08:13
@n1ru4l
Copy link
Collaborator

n1ru4l commented Feb 16, 2023

Currently, #8723 (comment), so it would be necessary to allow Plugin to write functions inline. I think a separate pull request for this would be appropriate. 🤔
If we don't use Plugin for transformDocuments, I will implement it in this pull request! 💪

As discussed here: #8723 (comment), let's allow passing a function.

That would then allow you to do something like the following:

import { type CodegenConfig } from '@graphql-codegen/cli';
import { addTypenameFieldSelections, addIdFieldSelection, inlineFragments } from '@wherever'

const config: CodegenConfig = {
  schema: './src/yoga.ts',
  documents: ['src/**/*.ts'],
  generates: {
    './src/gql/': {
      preset: 'client',
      transformDocuments: [addTypenameFieldSelections, addIdFieldSelection, inlineFragments]
    },
  },
};

export default config;

Copy link
Collaborator

@n1ru4l n1ru4l 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 the delayed answer. I added some comments, I am excited to get this close to getting merged!

@kazekyo
Copy link
Contributor Author

kazekyo commented Feb 19, 2023

@n1ru4l
I have introduced the concept of Document Transform and made changes to enable document transformation without using plugins. 🎉
In addition, I implemented the addToSchema function to resolve #8724.
Please refer to the documentation for details on how to use it.

Regarding the designs, please let me know if you have any feedback on the following points:

  1. documentTransforms is specified with objects and file names. It is also possible to implement a more simplified way of specifying it by directly writing the transform function, but I did not implement it as I did not need it. If you think it is necessary to make it more user-friendly, please let me know your feedback.
import type { CodegenConfig } from '@graphql-codegen/cli';

const config: CodegenConfig = {
  schema: 'https://localhost:4000/graphql',
  documents: ['src/**/*.tsx'],
  generates: {
    './src/gql/': {
      preset: 'client',
      documentTransforms: [
        ({ documents }) => { // Specify only the function
          return documents;
        },
      ],
    },
  },
};
export default config;
  1. The transform function and the addToSchema function have completely different required arguments depending on the case, so I am using destructured arguments to extract only the necessary arguments. Therefore, the addToSchema function has the same name as the function in the plugin, but the arguments are different. I chose to use the same function name to make it easier to understand how to use it, but if it causes confusion, I can change the function name to something like extendSchema.

If you notice anything else, please let me know. I'm confident the designs have improved through our discussions, and I welcome further feedback! 😃

@kazekyo kazekyo requested a review from n1ru4l February 19, 2023 09:47
@n1ru4l
Copy link
Collaborator

n1ru4l commented Feb 20, 2023

@kazekyo Can you give more context for the use case of addToSchema/extendSchema?

What is the use-case of coupling the schema transform with a specific document transform?

I think this is redundant with the following:

import type { CodegenConfig } from '@graphql-codegen/cli';

const config: CodegenConfig = {
  schema: ['https://localhost:4000/graphql', `extend type Query { foo: String }`], // see here
  documents: ['src/**/*.tsx'],
  generates: {
    './src/gql/': {
      preset: 'client',
    },
  },
};
export default config;

Would be happy, if we could immediately merge the already discussed stuff without the addToSchema part and then in a follow-up PR/issue discuss the necessity and use-cases for addToSchema. I want to ship the documentTransforms as soon as possible as we also want to use them. 😇

@kazekyo
Copy link
Contributor Author

kazekyo commented Feb 20, 2023

@n1ru4l

@kazekyo Can you give more context for the use case of addToSchema/extendSchema?

What is the use-case of coupling the schema transform with a specific document transform?

I think this is redundant with the following:

import type { CodegenConfig } from '@graphql-codegen/cli';

const config: CodegenConfig = {
  schema: ['https://localhost:4000/graphql', `extend type Query { foo: String }`], // see here
  documents: ['src/**/*.tsx'],
  generates: {
    './src/gql/': {
      preset: 'client',
    },
  },
};
export default config;

My use case is to implement Relay's __id. To do that, I need the addToSchema function which takes documents and schema as arguments. This is not possible with the current functionality. You can find more details in #8724.

Would be happy, if we could immediately merge the already discussed stuff without the addToSchema part and then in a follow-up PR/issue discuss the necessity and use-cases for addToSchema. I want to ship the documentTransforms as soon as possible as we also want to use them. 😇

If after reading the information I just provided, you still have additional discussion or questions regarding addToSchema, I will immediately remove addToSchema.
Afterward, I will either open another pull request or we can discuss further at #8724 if more discussion is needed!

@n1ru4l
Copy link
Collaborator

n1ru4l commented Feb 20, 2023

@kazekyo Let's keep addToSchema out of scope of this PR!

@kazekyo
Copy link
Contributor Author

kazekyo commented Feb 21, 2023

@n1ru4l I have removed addToSchema! 🚀

I don't think the failing test algolia/algolia-records-check is a problem with the code but please check it out if you need.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Feb 21, 2023

Thank you @kazekyo 🎉

@n1ru4l n1ru4l changed the title Add a feature to transform documents transform documents hooks Feb 21, 2023
@n1ru4l n1ru4l merged commit a3309e6 into dotansimha:master Feb 21, 2023
@kazekyo kazekyo deleted the document_transform branch February 22, 2023 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants