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

Why does codegen:generate require query names to be unique? #670

Closed
everdimension opened this issue Nov 8, 2018 · 19 comments
Closed

Why does codegen:generate require query names to be unique? #670

everdimension opened this issue Nov 8, 2018 · 19 comments

Comments

@everdimension
Copy link

everdimension commented Nov 8, 2018

It makes no sense.

I have a component with a query like this (simplified example):

const postsQuery = gql`
  query Posts {
    posts {
      id
      title
    }
  }
`;

And another componentn with a query like this:

const postsQuery = gql`
  query Posts {
    posts {
      id
      author
    }
  }
`;

When I run codegen:generate to generate typescript type, I get an error:

There can be only one operation named "Posts"

This forces me to think of unique query names for each query. But why? I don't need a unique query name here. The query is unique based on its content. The name is only helpful for debugging purposes.

And everything works fine. Apollo and graphql don't care about two queries having the same name. But I cannot generate types.

This seems to me to be horribly inconvenient. I want to colocate queries with my components. With this rule you could just as well disallow having two functions with the same name across the project.

Can this check be disabled somehow? Or do you think that my approach is wrong? If so, can you please try to explain the best course of action?

@lukepighetti
Copy link

lukepighetti commented Nov 18, 2018

It generates type names based on the query name, so I believe you would have type name collisions when outputFlat

@everdimension
Copy link
Author

@lukepighetti
I see, thanks. But it seems to me that this restriction should only exist for when outputFlat option is true.

When you're colocating queries to components it makes no sense. I want to have types, but I don't want to think of globally unique names for each query.

@warmbowski
Copy link

warmbowski commented Feb 6, 2019

I just ran into this too. Yeah, if I'm colocating the generated types near the component, I would like to opt out of the de-duplication of query names.

it took me a while to figure this out, as there was no output telling me that duplicates are ignored and only one component would get a type written. (apollo@2.4.4)

@JakeDawkins
Copy link
Contributor

Thanks for the thoughts, everyone! I've definitely thought about this a good bit.

As mentioned above, currently typenames are dependent on the operation name. While unique operation naming may not be a requirement in any way for GraphQL, I'd argue that it's also a best practice.

Naming operations has many more purposes than just a display name in code. Naming them uniquely makes them easier to find in a codebase, in debugging tools, etc. Not to mention the possibility of importing the wrong types when they're not unique. In addition to that, a lot of our current/future features rely on operation name for things like operation registries, metrics tracking, etc.

Right now, for those reasons, we don't have any plans to build in support for non-unique operation names, especially since the workaround is so simple.

I'm open to the idea of additional warnings and errors though, if that's what it takes to make these things easier. If anyone has any ideas, you can open an additional issue suggesting those. Hopefully that answers this 😄

@warmbowski
Copy link

That sounds reasonable, and I would think that 95% of the time the operation names would be unique anyway. I came across this in making a demo in which I was creating the same react component three different ways.

But it would be great to have a warning logged to the console like "Detected duplicate operation names, only one will have types generated", or something to that effect.

@ekfn
Copy link

ekfn commented Jul 16, 2019

@JakeDawkins are these necessary for near-operation-file plugin? There are no collisions in any files, although i still can't name simple query without prefixing it with context.

@ekfn
Copy link

ekfn commented Jul 16, 2019

I think unique names are good practice, but it shouldn't be mandatory. It's like react component's naming, with unique names it's easier to debug tree, but sometimes (especially for big applications) you can't guaranties unique names without starting to be too verbose, like NewPostsPageAuthorPhoto

@jorroll
Copy link

jorroll commented Jul 21, 2019

@JakeDawkins I'm using apollo codegen in a monorepo. Two separate apps in the same repo should be able to define their own version of GetUsers in the same way that two separate apps in different repos can define their own version of GetUsers. Requiring all naming to be unique in a repo, even when it doesn't make sense, leads to poor naming decisions (e.g. GetUsers1, GetUsers2, App1GetUsers, App2GetUsers).

Please reconsider your previous decision.

While unique operation naming may not be a requirement in any way for GraphQL, I'd argue that it's also a best practice.

While I agree this is true some of the time, this strikes me as a very opinionated stance for a tool that is simply generating types for a project. I didn't expect my type generation tool to enforce it's own (unspoken) naming conventions.

At minimum, Apollo codegen should output a warning whenever it skips generating types for a query. As @warmbowski said, the current behavior is unexpected and led to some confused debugging.

@JakeDawkins
Copy link
Contributor

@thefliik the current version of the CLI isn't meant to be used on multiple projects within the same repo at the same time. Each project in the monorepo should have their own apollo.config.js and the CLI should be run multiple times, once on each project. That'd solve what you're seeing with naming across projects.

As for warning, I don't disagree. But that should be opened as a separate issue. We're also pretty heavily backlogged on codegen issues at the moment, so I'm not sure how quickly something like this could be added/reviewed.

@jorroll
Copy link

jorroll commented Jul 21, 2019

the current version of the CLI isn't meant to be used on multiple projects within the same repo at the same time

@JakeDawkins gotcha. I created a separate issue to track the addition of a console warning for suppressed type generation #1427.

We're also pretty heavily backlogged on codegen issues at the moment

I also created a PR (#1428) as a "quick fix" to clarify usage of apollo codegen.

@jorroll
Copy link

jorroll commented Jul 21, 2019

For anyone else encountering this issue, the @graphql-codegen/cli package (which I just discovered while trying to work around this issue) has a less opinionated approach. It can generate typescript definitions for queries in different files that have the same name and it also supports usage in a monorepo.

@marekscholle
Copy link

marekscholle commented Sep 24, 2019

This is now a closed issue, so I'm not sure it is worth to comment it again. I think that forcing codegen clients to have unique operation names accross project is a bad decision as it goes against what makes GraphQL so great: you get (from server, cache, ...) only for what you really asked for.

A valid use case is that a developer (or developers) needs two almost (or by chance even exactly!) same queries in different modules (as mentioned in #670 (comment)) and writes these queries alongside code that uses them. Why should he invent two different names for operations?

A codegen client facing this problem has two options:

  • Invent different names, typically by adding ugly number suffixes (GetUser1, GetUser2) or pre/postfixing by some context (e.g. module name, GetUser_Logout)
  • merge similar queries to a single query and share among multiple modules. This is is the easiest and also the worst way as multiple modules will depend on single query which may gradually become unnecessary fat (you pay for what you get, remember?)

I fear that many codegen clients will go the latter way being "motivated" by arbitrary decision made by tool provider.

When working on larger project, two indendent developers may want the same/similar queries. They happily write queries for their needs, spawn independent PRs for different modules in project. But what if they chose the same names for their queries? When one of PRs is merged, the developer of the other has to change now-master code (and possibly ask for permission to change other team's code that just got merged) or invent worse name for his query.

I don't say that having option to force unique operations names is bad. Maybe it can be a default. But codegen needs to provide an opt-out to stay competitive.

There is still a problem (not mentioned above, surprisingly) how codegen should generate type definitions if two queries have the same name and are in the same folder (=> share __generated__ folder). In this case, it seems unavoidable to report this as error.

@dfelczak
Copy link

It's been a one year since last message and it seems like unsolved issue. Did anyone find workaround for this?

@jorroll
Copy link

jorroll commented Sep 11, 2020

See my comment above for a work-around.

@McPo
Copy link

McPo commented Nov 27, 2020

I agree with @marekscholle. Its particularly annoying when the queries are exactly the same (and thus I wish to use the same name), and do not wish to place them into a separate module for the same reasons that @marekscholle pointed out. (Using VUE SFC, so its nice to the structure of the data you are getting back within the same component).

As such I would propose that the query check should take in to account more than just the name, if the query is exactly the same it should not throw this error (As theres no conflict, just duplication).

On another point, is this also the same reason why Anonymous names are not allowed? I have noticed the VSCode Apollo GraphQL extension complains about Apollo not supporting anonymous names. However it appears to work fine, Unfortunately there does not appear to be a way to suppress the extension from flagging this as an issue with the editor.

@cblaettl
Copy link

cblaettl commented Dec 8, 2020

a workaround:

assuming the following project structure

├── scripts
│   └── generate-types.sh      <-- script goes here
├── node_modules
├── public
└── src
    ├── assets
    ├── module-1
    │   ├── ...
    │   ├── queries
    │   ├── mutations
    │   └── apollo.config.js   <-- note the separate apollo.config.js per module
    └── module-2
        ├── ...
        ├── queries
        ├── mutations
        └── apollo.config.js   <-- note the separate apollo.config.js per module

you can generate types by adding the following line to your package.json and copying the script below to /scripts.

{
  ...
  "scripts": {
    ...
    "generate": "./scripts/generate-types.sh src/"
  },
}

usage

npm run generate

generate-types.sh:

#!/bin/bash
set -euo pipefail

echo $PWD

cd $1

echo $PWD

for dir in */
do
    dir=${dir%*/}      # remove the trailing "/"
    cd $dir

    config=apollo.config.js

    if test -f "$config"; then
      echo "$dir/ contains $config file, generating types."

      npx apollo client:codegen generated \
        --config=apollo.config.js \
        --target=typescript \
        --globalTypesFile=../../src/global-types/$dir.ts \
        --passthroughCustomScalars \
        --useReadOnlyTypes
    else 
      echo "$dir/ contains no $config file, skipping."
    fi

    cd ..
done

drawbacks:

  • global types are added per module and not in one file

@sergnio
Copy link

sergnio commented Jul 16, 2021

@marekscholle is right. What makes GraphQL so powerful is that you can request exactly what you want, even if it's the same endpoint.

I have a use case where I want to call query getHistoryById, one time with many parameters, and another time with only a few parameters.

I would be fine using the bigger query except that it takes several seconds to run with many parameters, so I don't want to call the larger query when I can call a smaller query for the same endpoint that takes milliseconds to run.

But I can't with this current implementation of Apollo.

I'm sure I'm not the only person in 2021 experiencing this issue so I would strongly encourage you to take a second look at this.

@stelmakhivan
Copy link

Each operation in document must be unique (if follow GraphQL specification)
GraphQL Specification

@marekscholle
Copy link

@stelmakhivan Yes, but this is kind of unrelated to the discussion above. The problem would someone use two queries with the same name in the same document, in other words in single HTTP request / response.

By enforcing different names we can enforce this can't happen, but IMO the cost would be two high.
And if we assume common usage, it's not foreseen some client would import generated code for two queries inside same module

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

No branches or pull requests