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

fix(base-entity): set create return type to T[] #5400

Merged
merged 1 commit into from
Feb 14, 2020
Merged

fix(base-entity): set create return type to T[] #5400

merged 1 commit into from
Feb 14, 2020

Conversation

merceyz
Copy link
Contributor

@merceyz merceyz commented Jan 22, 2020

What's the problem this PR addresses?

The version of BaseEntity.create that takes an array of entityLike objects returns an array of entities but the return type incorrectly specifies that it's a single entity.

How did you fix it?

Changed the return type from T to T[]

@pleerock pleerock merged commit ceff897 into typeorm:master Feb 14, 2020
@pleerock
Copy link
Member

Thanks!

@merceyz merceyz deleted the patch-1 branch February 14, 2020 14:50
@jalooc
Copy link

jalooc commented Mar 7, 2020

FYI this broke existing code in a way described in this issue: #1583 (comment)

tl;dr; this code:

const foo = Foo.create({ ...req.body }) // req.body is `any`
console.log(foo.bar)

suddenly breaks, because before the update foo used to be of Foo type and after the update it becomes Foo[] (note the array).

This might be TS fault instead TypeORM's fault, I'm not sure. But since it's breaking working code, it definitely requires a second look.

@sherbakovdev
Copy link

Same for me. This broke the existing code.

@Diluka
Copy link
Contributor

Diluka commented Mar 20, 2020

I thought it was TypeScript(microsoft/TypeScript#37471), but turned out to be this.

@itsjbecks
Copy link

Has there been any update on this / more background on why this was changed. It also broke existing code on our project.

@Diluka
Copy link
Contributor

Diluka commented Mar 25, 2020

@pleerock look here. this pr introduce a breaking change

@pleerock
Copy link
Member

Type signature seems to be right. It expects an array and we get array back. I don't see how it can be wrong right now, can somebody point me what can be wrong ?

@jalooc
Copy link

jalooc commented May 18, 2020

@pleerock The code looks good, but its usage is crooked. The problem is that when using object spread operator in the create({ ...x }) argument, TS somehow thinks (probably because of some TS error) that this definition should be used, not that. Therefore doing something like this: const y = Model.create({ ...y }) makes y of type Model[] instead of Model, which was not the case before this PR. So while what you wrote here seems totally logical, it does not work this way.

@pleerock
Copy link
Member

okay, but this PR definitely fixes issue. Not sure how can I help with particular this use case when you use any object.

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

Successfully merging this pull request may close these issues.

None yet

6 participants