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

Input logic in 0.18.0-beta.4 #510

Closed
joemckie opened this issue Jan 4, 2020 · 5 comments
Closed

Input logic in 0.18.0-beta.4 #510

joemckie opened this issue Jan 4, 2020 · 5 comments
Labels
Question ❔ Not future request, proposal or bug issue Solved ✔️ The issue has been solved

Comments

@joemckie
Copy link

joemckie commented Jan 4, 2020

Since 0.18.0-beta.4, the logic for inputs (although only nested inputs, strangely) seems to have changed, resulting in Firestore throwing errors when using their data directly (Firestore only takes plain objects, not instantiated classes)

I've managed to work around it by using classToPlain() as Type, but doing this for each instance of an input is quite messy. I'm wondering if this is intended behaviour?

@MichalLytek
Copy link
Owner

I'm wondering if this is intended behaviour?

Yes, TypeGraphQL always transform the plain args into classes instance according to the shapes of Input Types.
In previous version there was a bug and only a root-level args/inputs were transformed, now all of them.

(Firestore only takes plain objects, not instantiated classes)

What do you mean? Can you show your input type classes?

@MichalLytek MichalLytek added Need More Info 🤷‍♂️ Further information is requested Question ❔ Not future request, proposal or bug issue labels Jan 4, 2020
@joemckie
Copy link
Author

joemckie commented Jan 4, 2020

What do you mean? Can you show your input type classes?

Here is a sample:

@InputType()
export class OrderInput implements Partial<Order> {
  @Field(_type => OrderCustomerInput, { nullable: true })
  public partialCustomer?: OrderCustomerInput;

  @Field(_type => OrderStatus)
  public status: OrderStatus;

  @Field(_type => [OrderLineItemInput])
  public lineItems: OrderLineItemInput[];
}

After doing some digging, it seems like this is limited only to nested arrays.

In this instance, lineItems throws an error as the array contains classes that need converting, however as other properties are simple types, they are mapped 1-to-1 (I presume spreading a class applies its properties onto the new object, however the array maintains the internal classes).

If this is intended behaviour there's probably not much to be done, I just wanted to check and raise an issue in case anyone else came across this behaviour. I can see the benefits of using classes when performing validation on inputs 😄

@MichalLytek
Copy link
Owner

this is limited only to nested arrays.
lineItems throws an error as the array contains classes that need converting

What is limited? Why it is throwing an error?

I can see the benefits of using classes when performing validation on inputs

You mean that earlier you passed wrong data and now the validation works and it throws validation errors? 😄

@joemckie
Copy link
Author

joemckie commented Jan 4, 2020

What is limited? Why it is throwing an error?

It's limited because Firestore doesn't accept classes, only plain objects. When I insert into the database, I spread the input, which converts it into a plain object, but any nested properties don't get converted automatically.

You mean that earlier you passed wrong data and now the validation works and it throws validation errors? 😄

Not quite 😉before, type-graphql didn't instantiate nested inputs so it worked fine. Now it's changed, I need to convert those manually.

I'm going to close this issue, I don't think there's anything wrong with the code!

@joemckie joemckie closed this as completed Jan 4, 2020
@MichalLytek
Copy link
Owner

It's limited because Firestore doesn't accept classes, only plain objects.

All right! So Firebase is the proper guilty one 😄
firebase/firebase-js-sdk#311

@MichalLytek MichalLytek added Solved ✔️ The issue has been solved and removed Need More Info 🤷‍♂️ Further information is requested labels Jan 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question ❔ Not future request, proposal or bug issue Solved ✔️ The issue has been solved
Projects
None yet
Development

No branches or pull requests

2 participants