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

Cannot return plain objects when using inherited ObjectTypes #160

Closed
cuchi opened this issue Oct 1, 2018 · 8 comments · Fixed by #279
Closed

Cannot return plain objects when using inherited ObjectTypes #160

cuchi opened this issue Oct 1, 2018 · 8 comments · Fixed by #279
Labels
Bug 🐛 Something isn't working
Projects
Milestone

Comments

@cuchi
Copy link

cuchi commented Oct 1, 2018

Describe the bug
When I'm using inheritance between @ObjectType classes, I cannot return anything that is not an valid instanceof MyInheritedObjectType. This does not happen when I use just plain, non-inherited classes.

To Reproduce
There is this gist

Expected behavior
In the gist I made, query { users } plays just nice with the plain objects array I provided. TS also statically checks it with no problem.
When calling query { inheritedUsers }, I expect it to have the same behavior, but it instead fails because the array does not meet the correct type.

Logs

{
  "data": null,
  "errors": [
    {
      "message": "Expected value of type \"InheritedUser\" but got: [object Object].",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "complexUsers",
        0
      ],
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR",
        "exception": {
          "message": "Expected value of type \"InheritedUser\" but got: [object Object].",
          "locations": [
            {
              "line": 2,
              "column": 3
            }
          ],
          "stacktrace": [
            "Expected value of type \"InheritedUser\" but got: [object Object].",
            "",
            "GraphQL request (2:3)",
            "1: {",
            "2:   inheritedUsers {",
            "     ^",
            "3:     id",
            "",
            "    at invalidReturnTypeError (/home/paulo/src/type-graphql-test/node_modules/graphql/execution/execute.js:766:10)",
            "    at completeObjectValue (/home/paulo/src/type-graphql-test/node_modules/graphql/execution/execute.js:758:13)",
            "    at completeValue (/home/paulo/src/type-graphql-test/node_modules/graphql/execution/execute.js:660:12)",
            "    at completeValue (/home/paulo/src/type-graphql-test/node_modules/graphql/execution/execute.js:629:21)",
            "    at completeValueWithLocatedError (/home/paulo/src/type-graphql-test/node_modules/graphql/execution/execute.js:580:21)",
            "    at completeValueCatchingError (/home/paulo/src/type-graphql-test/node_modules/graphql/execution/execute.js:550:12)",
            "    at /home/paulo/src/type-graphql-test/node_modules/graphql/execution/execute.js:684:25",
            "    at Array.forEach (<anonymous>)",
            "    at forEach (/home/paulo/src/type-graphql-test/node_modules/iterall/index.js:83:25)",
            "    at completeListValue (/home/paulo/src/type-graphql-test/node_modules/graphql/execution/execute.js:680:24)"
          ]
        }
      }
    }
  ]
}

Enviorment

  • OS: Debian Sid
  • Node v8.11.3
  • Package version 0.14.0
  • TypeScript version 3.1.1

Additional context
This could easily be fixed by removing this verification, but I'm pretty sure that is here for a reason, and I could break somewhere else by removing it.

@MichalLytek MichalLytek added the Question ❔ Not future request, proposal or bug issue label Oct 1, 2018
@MichalLytek
Copy link
Owner

This requirement takes place when the return type is union or your object types implements GraphQL interfaces or extends other classes. One of the super class might be implementing an GraphQL interface and unfortunately due to a limitation of current schema generation using graphql-js, I'm not able to detect it during ObjectType generation in schema.

@MichalLytek
Copy link
Owner

Reopening as it should be fixed along with metadata storage refactoring 🔓

@MichalLytek MichalLytek reopened this Oct 22, 2018
@MichalLytek MichalLytek added Bug 🐛 Something isn't working and removed Question ❔ Not future request, proposal or bug issue labels Oct 22, 2018
@MichalLytek MichalLytek added this to Backlog in Board via automation Oct 22, 2018
@MichalLytek MichalLytek added this to the 1.0.0 release milestone Oct 22, 2018
@MichalLytek MichalLytek moved this from Backlog to To Do in Board Oct 22, 2018
@hcharley
Copy link

I think I am experiencing this as well.

@maxprilutskiy
Copy link

maxprilutskiy commented Jan 17, 2019

+1. Highly useful feature, in terms of reducing code duplication. Can't wait to extract some fields to a BaseModel ObjectType:

@ObjectType()
class BaseModel {
  @Field(() => String)
  public id: string;

  @Field(() => String)
  public createdAt: string;

  @Field(() => String)
  public updatedAt: string;
}

@MichalLytek
Copy link
Owner

MichalLytek commented Jan 17, 2019

@prilutskiy
I also plan to add the mix(Foo).with(Bar) approach to support kinda multiple inheritance:
https://github.com/justinfagnani/mixwith.js

@vcfvct
Copy link

vcfvct commented Mar 12, 2019

Having the same issue with inheritance today. Type converter is a bit heavy(for big array) and not elegant.
Hopefully this to be fixed soon. Do we roughly have a timeline for this? 😃
@19majkel94 Thanks!

@MichalLytek
Copy link
Owner

@vcfvct
Acutally, it was easier than I thought 😄 #279 🚀

@MichalLytek MichalLytek moved this from To Do to In review in Board Mar 13, 2019
@vcfvct
Copy link

vcfvct commented Mar 14, 2019

@19majkel94 , You the man! 👍 👍
As I mentioned previously, would you mind let me know a rough time you are releasing this fix so that we can also plan ahead? Thanks a lot! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 Something isn't working
Projects
Board
  
Done
Development

Successfully merging a pull request may close this issue.

5 participants