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

Comments

Projects
5 participants
@cuchi
Copy link

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.

@19majkel94

This comment has been minimized.

Copy link
Owner

commented Oct 1, 2018

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.

@19majkel94 19majkel94 closed this Oct 1, 2018

@19majkel94

This comment has been minimized.

Copy link
Owner

commented Oct 22, 2018

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

@19majkel94 19majkel94 reopened this Oct 22, 2018

@19majkel94 19majkel94 added Bug 🐛 and removed Question labels Oct 22, 2018

@19majkel94 19majkel94 added this to Backlog in Board via automation Oct 22, 2018

@19majkel94 19majkel94 added this to the 1.0.0 release milestone Oct 22, 2018

@19majkel94 19majkel94 moved this from Backlog to To Do in Board Oct 22, 2018

@charlex

This comment has been minimized.

Copy link

commented Nov 13, 2018

I think I am experiencing this as well.

@prilutskiy

This comment has been minimized.

Copy link

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;
}
@19majkel94

This comment has been minimized.

Copy link
Owner

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

This comment has been minimized.

Copy link

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!

@19majkel94

This comment has been minimized.

Copy link
Owner

commented Mar 13, 2019

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

@19majkel94 19majkel94 moved this from To Do to In review in Board Mar 13, 2019

@vcfvct

This comment has been minimized.

Copy link

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! 😃

@19majkel94 19majkel94 closed this Mar 16, 2019

@19majkel94 19majkel94 moved this from In review to Done in Board Mar 16, 2019

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