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.

Sign up for GitHub

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

Jump to bottom

TS method overloads need to be re-ordered #11726

Closed
2 of 7 tasks
dancrumb opened this issue Dec 3, 2019 · 2 comments
Closed
2 of 7 tasks

TS method overloads need to be re-ordered #11726

dancrumb opened this issue Dec 3, 2019 · 2 comments
Labels
status: wip For issues and PRs. Applied when the PR is not ready yet / when work to close the issue has started. type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.

Comments

@dancrumb
Copy link
Contributor

dancrumb commented Dec 3, 2019

Issue Description

The method overloading order is incorrect for a number of Model methods. This results in the wrong method signature being returned.

What are you doing?

I'm passing a NonNullFindOptions object into Model.findOne.

Here is the link to the SSCCE for this issue: TS Playground

What do you expect to happen?

I expect to get a Promise<Model> back.

What is actually happening?

I get a Promise<Model | null> instead.

Additional context

The TS handbook outlines that

... it’s customary to order overloads from most specific to least specific.

Environment

  • Sequelize version: 5.21.2
  • Node.js version: 12.13.0
  • Operating System: macOS Catalina
  • If TypeScript related: 3.7.2

Issue Template Checklist

How does this problem relate to dialects?

  • I think this problem happens regardless of the dialect.
  • I think this problem happens only for the following dialect(s):
  • I don't know, I was using PUT-YOUR-DIALECT-HERE, with connector library version XXX and database version XXX

Would you be 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, although I believe I could do it if I had the time...
  • No, I don't have the time and I wouldn't even know how to start.
dancrumb added a commit to dancrumb/sequelize that referenced this issue Dec 3, 2019
@dancrumb
Fixes sequelize#11726 - correct the overloaded method order
c1e87cd 
When methods are overloaded, such that the different parameters are extensions of each other, the most specific signatures must come first.
@dancrumb dancrumb mentioned this issue Dec 12, 2019
Merged
@papb papb added status: wip For issues and PRs. Applied when the PR is not ready yet / when work to close the issue has started. type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. labels Jan 16, 2020
@papb
Copy link
Member

papb commented Jan 16, 2020

Just linking the relevant discussion here: microsoft/TypeScript#35472

@papb
Copy link
Member

papb commented Jan 16, 2020

Fixed by #11727

@papb papb closed this as completed Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
status: wip For issues and PRs. Applied when the PR is not ready yet / when work to close the issue has started. type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dancrumb @papb