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

getRawOne return type ought to include undefined #7449

Closed
1 of 4 tasks
osdiab opened this issue Mar 5, 2021 · 2 comments · Fixed by #7863
Closed
1 of 4 tasks

getRawOne return type ought to include undefined #7449

osdiab opened this issue Mar 5, 2021 · 2 comments · Fixed by #7863

Comments

@osdiab
Copy link
Contributor

osdiab commented Mar 5, 2021

Documentation Issue

What was unclear or otherwise insufficient?

the typings for getRawOne allow any type parameter T and blindly returns a Promise<T>, so if you use it and no matching db row is found, it will happily lie to you and say that the row is present via the type system. this isn't wrong, per se, but it's an easy footgun.

Recommended Fix

make the typing of getRawOne return the parameterized type | undefined, in case there are no rows present. users of typeorm can use the non-null assertion syntax if they really think it's present.

Additional Context

my company's app had unexpected 500s from this, but we're used to TypeORM making undefined access errors go away, because that's something it's particularly good at, if your typings don't do unsafe things like this.

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, 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.
@macksal
Copy link

macksal commented Mar 6, 2021

Simplest fix would be to change the return type to Promise<T> | undefined in this file and fixing any type errors which propagate from there.

In the long-term I think it's best to enable tsconfig's noUncheckedIndexAccess. It's a great feature to prevent issues like this. For context it makes an index access to a value of type T[] return T | undefined. In this case, that would generate an error with the type signature of getRawOne.

@imnotjames imnotjames self-assigned this Jul 4, 2021
@imnotjames
Copy link
Contributor

imnotjames commented Jul 8, 2021

It probably shouldn't even be Promise<T | undefined> but.. that's a separate issue. - oh wait, T is definable as a generic on the function. That makes sense.

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

Successfully merging a pull request may close this issue.

3 participants