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

DeepPartial simplification breaks the .create() and .save() method in certain cases. #8681

Closed
pbrn46 opened this issue Feb 22, 2022 · 11 comments · Fixed by #8732 or #8817
Closed

DeepPartial simplification breaks the .create() and .save() method in certain cases. #8681

pbrn46 opened this issue Feb 22, 2022 · 11 comments · Fixed by #8732 or #8817

Comments

@pbrn46
Copy link
Contributor

pbrn46 commented Feb 22, 2022

Issue Description

After the DeepPartial simplication of #8187, TypeScript is no longer able to match certain situations using the .create() and .save() functions. Specifically, when we are trying to pass a relation that is nested within a deep partial entity instance which is also an array, it will no longer match the DeepPartial<Entity>[] type signature and fail. See example code.

Steps to Reproduce

@Entity()
class Item {
  @PrimaryGeneratedColumn()
  id!: number

  @ManyToOne(() => Thing, thing => thing.items)
  thing!: Thing
}

@Entity()
class Thing {
  @PrimaryGeneratedColumn()
  id!: number

  @OneToMany(() => Item, item => item.thing)
  items!: Item[]
}


export async function test() {
  const myThing: DeepPartial<Thing> = {
    items: [{ id: 1 }, { id: 2 }]
  }

  const thing = getManager().create(Thing, myThing) // OK
  await getRepository(Thing).save(myThing) // OK

  const items = getManager().create(Item, myThing.items) // Not OK, myThing.items is of type `(Item | undefined)[] | undefined`
  // - or -
  await getRepository(Item).save(myThing.items) // Not OK for the same reason as above.
}

My Environment

Dependency Version
Operating System
Node.js version 4.17.5
Typescript version 4.5.5
TypeORM version 0.2.43

Are you willing to resolve this issue by submitting a Pull Request?

  • ✅ Yes, I have the time, and I know how to start.
  • ✖️ Yes, I have the time, but I don't know how to start. I would need guidance.
  • ✖️ No, I don’t have the time, but I can support (using donations) development.
  • ✖️ No, I don’t have the time and I’m okay to wait for the community / maintainers to resolve this issue.
@H4ad
Copy link

H4ad commented Mar 1, 2022

Related #8698

@rodericktech
Copy link

Just commenting to say I have gotten a similar error and we have held off from updating while waiting to see if this can be addressed relatively quickly - in one of our projects the BaseRepository (abstract class) can no longer compile (post-0.2.41) while 'entity' below is typed 'T':

public async save(entity: T): Promise<T> { return this.repository.save(entity); }

The error returned by the compiler when updating to any more recent version of typeorm is as follows:

Type 'DeepPartial<T>[]' is not assignable to type 'T'. 'T' could be instantiated with an arbitrary type which could be unrelated to 'DeepPartial<T>[]'.

The softRemove() call breaks in the same way.

Both errors are temporarily resolved by casting 'entity' to any instead of T but that doesn't seem like a great (or safe) long-term solution for a complex project.

pleerock added a commit that referenced this issue Mar 26, 2022
* fix: improve DeepPartial recursion

Closes: #8681

* type simplification

* merging master

* format

* format

Co-authored-by: Umed Khudoiberdiev <pleerock.me@gmail.com>
@nelsonfleig
Copy link
Contributor

I've upgraded to 0.3.4 and still get the message from the compiler:

Type 'Promise<DeepPartial<T>[]>' is not assignable to type 'Promise<T>'.
  Type 'DeepPartial<T>[]' is not assignable to type 'T'.
    'T' could be instantiated with an arbitrary type which could be unrelated to 'DeepPartial<T>[]'.ts(2322)

In my AbstractService:

  create(data: DeepPartial<T>): Promise<T> {
    const entity = this.repository.create(data);
    return this.repository.save(entity);
  }

@pleerock
Copy link
Member

@nelsonfleig provide a PR containing failing test with minimal reproduction code and I'll check that for you.

@nelsonfleig
Copy link
Contributor

@pleerock Created PR #8817

@nelsonfleig
Copy link
Contributor

I've been working with abstract services similar to @rodericktech before 0.3.0 and had no issues with pretty much the same code as in the unit test. The issue seems to arise when passing a generic to the Repository class. When creating and saving with a repository instance using getRepository(), this is not an issue.

M-TGH pushed a commit to TradeCast/typeorm that referenced this issue Mar 29, 2022
* fix: improve DeepPartial recursion

Closes: typeorm#8681

* type simplification

* merging master

* format

* format

Co-authored-by: Umed Khudoiberdiev <pleerock.me@gmail.com>
@krystalmonolith
Copy link
Contributor

I have basically the same issue with my generic wrapper I've written around code based on v0.2.41. As of 3/30/2022 all TypeORM versions starting with 0.2.42 and above show the same issue within the Repository API method signatures that use DeepPartial<Entity>. This is a significant bummer because I spent weeks refactoring into my generic wrapper and post 0.2.41 the errors produced by the TypeScript complier are incredibly hard to decipher. It seems that most of my problems relate back to the change Improve DeepPartial using recursive types #8187 in that the recursive nature of DeepPartial causes circularity compilation errors when trying to extend my ENTITY generic type to ENTITY extends DeepPartial<ENTITY> which seems the simplest path to fixing the create/save issues noted above by @rodericktech. Since my entire project is based off of NestJS I may bail on TypeORM completely and move to MongoDB & Mongoose.

PS: @pleerock .et .al... Don't take this personally but I hate ORMs. All of them.

@pbrn46
Copy link
Contributor Author

pbrn46 commented Mar 31, 2022

@krystalmonolith #8817 should fix the generic issues if merged. No need to cast to any. You can test out the changes there ahead of time if you'd like.

@pleerock
Copy link
Member

pleerock commented Apr 2, 2022

I may bail on TypeORM completely and move to MongoDB & Mongoose.
PS: @pleerock .et .al... Don't take this personally but I hate ORMs. All of them.

@krystalmonolith even if it's not something related to TypeORM, I have to say your statements are confusing. Mongoose also can be treated as ORM. Decision of switching from one database type to another depend on the project requirements, it's weird to do this decision based on some personal preferences until the project isn't something serious in terms of storage requirements. Regarding to the problems you have overall... well, it happens when you rely on other people code / libraries. But:

  • more dependencies you have -> more problems are solved for you -> less you have to do -> but increased probability of such issues
  • less dependencies you have -> more problems you have to solve on your own -> no issues with third-party written code -> more issues with your code and more things you have to maintain

ORM is just an abstraction layer that does some work for you, and as in everything there is no silver bullet. It doesn't work perfectly in all cases, but can work in most cases. If programmer choose a wrong tooling for his task - it's his fault, no need to hate tooling and create irrational holy wars.

@pbrn46
Copy link
Contributor Author

pbrn46 commented Apr 2, 2022

I have tried quite of few ORMs in-depth, and I have to say that TypeORM has been by far the best experience for a Typescript project. With the many ORMs, I've always hit a brick wall one way or another. Creating workarounds for some other ORMs have been quite painful, but it's generally simple enough to work around for complex issues with TypeORM. Using the query builder is very helpful, yet I've actually been using some of the undocumented exports with great success since the internal code is easy enough to understand. Even for the DeepPartial, I've been using my own implementation instead of TypeORM before the latest pull request. It's still better than writing my own ORM from scratch or using none at all.

ORMs are an abstraction layer, but to be honest, we all hope to use it as a simplification layer. The problem quickly becomes "I want to use this package to simplify the workflow, but I want it to do things my way. Another stance one would take is "I want there to be no problems because it interferes with my own work". Well, welcome to open source in general. Either contribute ideas or help test.

ORMs are here to add helpful features and save us all time and hair loss, but nobody says we can't drop down to raw SQL while using an ORM. Even if we program our own adapter to convert from SQL to JS objects in our preferred format, isn't that some form of ORM anyway? If we do program our own "ORM", should we then hate our own implementations of an ORM as well? We might as well use something robust and already out there to save us all some time.

It saddens me that there is such hate for ORMs, or hate in general. I want to say these things to counteract some of this negative energy to help propel this community in a positive direction. This project has been amazing and I hope it continues to progress and mature, even if there are breaking changes! @pleerock this project is amazing! And congrats on breaking 1 million weekly NPM downloads!

@nubunto
Copy link

nubunto commented Sep 15, 2023

still happening. I added a new field to an entity, and it breaks with the following:

Argument of type 'Payment' is not assignable to parameter of type '_QueryDeepPartialEntity<Payment> | _QueryDeepPartialEntity<Payment>[]'.
  Type 'Payment' is not assignable to type '_QueryDeepPartialEntity<Payment>'.
    Types of property 'events' are incompatible.
      Type 'Event[]' is not assignable to type '(() => string) | _QueryDeepPartialEntity<Event>[] | undefined'.
        Type 'Event[]' is not assignable to type '_QueryDeepPartialEntity<Event>[]'.
          Type 'Event' is not assignable to type '_QueryDeepPartialEntity<Event>'.
            Types of property 'payment' are incompatible.
              Type 'Payment' is not assignable to type '_QueryDeepPartialEntity<Payment> | (() => string) | undefined'. [2345]

funny thing is: I didn't even touch the Event field. I added another field, that is @OneToOne and @JoinColumn. If I remove this new field, the error goes away.

code is something along these lines:

  const result = this.paymentRepository.create({
      ...payment,
      id: this.newPaymentId(),
      value: this.valueWithDefaultPrecision(payment.value),
      status: 'created',
    });
    await this.paymentRepository.insert(result);
    return result.id;

changing it to

await this.paymentRepository.insert({
    ...payment,
    id: this.newPaymentId(),
    value: this.valueWithDefaultPrecision(payment.value),
    status: 'created',
});

works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants