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

CLI codegen ergonomics #134

Closed
tomhoule opened this issue Sep 20, 2018 · 11 comments
Closed

CLI codegen ergonomics #134

tomhoule opened this issue Sep 20, 2018 · 11 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@tomhoule
Copy link
Member

tomhoule commented Sep 20, 2018

At the moment the CLI generates one module for one file. The general use-case however is probably projects with more than one query, and having to issue more than one command seems a bit too complicated.

The most use similar CLI tool at the moment is apollo-cli and I think we could take a similar approach.

Invoking graphql-client-cli could look something like this:

graphql-client-cli generate --schema-path=./schema.json --deprecations=warn 'src/**/*.graphql', and it would generate one file per .graphql file, with one module per query.

The shared code could live in one module for the whole project - we can achieve a lot more reuse with this approach than with the derive-based interface.

Let's discuss what interface we want for the CLI in this isue.

@tomhoule tomhoule added enhancement New feature or request help wanted Extra attention is needed labels Sep 20, 2018
@tomhoule
Copy link
Member Author

@h-michael tagging you since you implemented the CLI codegen - in case you want to discuss this.

@h-michael
Copy link
Member

I agree with this idea.
But only one thing to consider is how to handle the --additional - derives option.
Although it may be a trivial problem, some people may want to divide what they want to derive.
If the policy is decided, I would like to start implementing samples that divide multiple modules into one file.

@tomhoule
Copy link
Member Author

tomhoule commented Sep 21, 2018

There is one library with a similar problem (although it's used as a build script): prost.

Relevant doc page (in particular the field_attribute and type_attribute functions). I don't find the solution very elegant and would like to find something better if we can, but it works.

@tomhoule
Copy link
Member Author

One other solution we could explore is giving options like additional_derives as annotation or directives inside the .graphql files.

@h-michael
Copy link
Member

One other solution we could explore is giving options like additional_derives as annotation or directives inside the .graphql files.

Even if it is an annotation, I do not think it is a good thing to describe what is not in the specification.
If we do not provide it with the option of cli, is not it better to provide a config file for graphql-client-cli?

For example, how about this?

graphql-client-cli generate --schema-path=./schema.json --deprecations=warn 'src/**/*.graphql' \
  --common-additional-derives='Debug,Clone' \
  --additional-derives='FooTrait:TargetStruct1,TargetStruct2'

The format of the option needs to be discussed, but in essence it separates the options of deriving in common and deriving only for a specific struct.

However, as I think that it is complicated as cli, I think that it may be possible to provide a config of toml format that can handle the same format.
I think that it is more convenient to have config if it does not execute the generate command only once in one project.

@tomhoule
Copy link
Member Author

I agree with the config, and that we should have both config and options. There is a standard for the prisma-related graphql tooling, graphqlconfig, but I am not sure it would make more sense than using our own format.

@h-michael
Copy link
Member

I'll investigate graphql-config format later.

@tomhoule
Copy link
Member Author

I think toml might be a better target - users of the library will be used to Rust and most Rust projects prefer toml (easier to read and edit, supports comments, etc.). Comparatively few people know graphqlconfig and it would be mostly custom options anyway. But I am interested to know what you think.

@h-michael
Copy link
Member

I will start implementing building all queries in the same module before config.

@h-michael
Copy link
Member

@tomhoule
I think that both of derive and codegen generate code including all queries defined as default is better.
But this is breaking change.
How do you think about this?

@tomhoule
Copy link
Member Author

@h-michael 's last suggestion was implemented in 0.7.0. Closing this issue in favour of more focused ones to gather new feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants