Remove more instances of as unknown as

[koesie10]

See the individual commits for the full changes. This removes as unknown as for some more types.

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[elenatanasoiu]

If we undefine then for all mocked objects, then their behaviour might be different than their original counterpart.

Would we be able to check if the original object is thenable?

• If it is, we could return a mocked method for then (or allow the user to pass in a mocked method? that might be a stretch)
• If it isn't, return undefined like you've done here.

What do you think?

[koesie10]

I think it would be nice if we could detect whether something is thenable, but unfortunately I don't think we're able to do that. TypeScript doesn't expose the type information at runtime, so we can't get access to the original object. The only way we could do it is by passing in a boolean that matches whether the object is thenable or not.

However, I don't see us mocking a thenable object anytime soon. The only thenable object which is used in our codebase is Promise and this should never be mocked since it can be constructed really easily using Promise.resolve or Promise.reject. I would be in favour of not supporting this usecase and leaving the code as-is, always returning undefined.

[elenatanasoiu]

The only thenable object which is used in our codebase is Promise and this should never be mocked since it can be constructed really easily using Promise.resolve or Promise.reject.

It looks like there are a few more cases where we use Thenable objects: https://github.com/github/vscode-codeql/search?p=2&q=thenable.

Also other libraries we're using could return Thenable objects. When we try to mock them, we might not be aware of the behaviour change.

However, I don't see us mocking a thenable object anytime soon.

Yeah, I'm more thinking how this isn't a visible thing on mock objects and could therefore affect us without us realising it.

[koesie10]

It looks like there are a few more cases where we use Thenable objects: https://github.com/github/vscode-codeql/search?p=2&q=thenable.

These are references to the Thenble interface which defines that there must be a then method on some object. It is not an implementation of this interface and in almost all cases it will be a Promise.

Also other libraries we're using could return Thenable objects. When we try to mock them, we might not be aware of the behaviour change.

If we're mocking other libraries, we should still be using mockResolvedValue which will return a Promise. Even other libraries shouldn't implement a thenable object and should be using Promise. I don't see a reason for a library to re-implement a promise, other than for compatibility with older JS versions which didn't support promise. However, even in that case we would still be using mockResolvedValue.

Yeah, I'm more thinking how this isn't a visible thing on mock objects and could therefore affect us without us realising it.

I think the risk here is very low, you'd need to be intentionally mocking a Promise or Thenable using something like mockedObject<Promise<AnotherObject>> or mockedObject<Thenable<AnotherObject>>. This would only work if you also pass an empty object to the mockedObject method since you wouldn't be able to mock any methods of AnotherObject (those will give type errors).

I'm open to suggestions on how we can make this work with promises, but I think it will unnecessarily complicate things because we can't actually detect whether something is thenable and would like to avoid adding another parameter which is always false.

[elenatanasoiu]

Yeah, I'm not sure I can suggest a better workaround tbh. Just wanted to understand what the implications are, which you've laid out clearly. 👍

I wasn't familiar with the differences between Thenable and Promise. After reading your explanation, it makes sense that modern libraries would be using Promises.

And we do mock promises intentionally to return what we want, so we have an escape hatch.