Skip to content

Expose getUnknownType #56826

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

Closed

Conversation

sstchur
Copy link
Contributor

@sstchur sstchur commented Dec 18, 2023

Fixes #

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Dec 18, 2023
@jakebailey
Copy link
Member

This makes sense, but do you have some context for this?

@sstchur
Copy link
Contributor Author

sstchur commented Jan 5, 2024

@jakebailey, Sure, I can provide context. Jason Gore was doing some work where he was writing an ESLint rule and needed access to the unknown type. I recalled that I had done the work to expose getNumberLiteralType and getStringLiteralType and it occurred to me that perhaps getUnknownType should have been a part of that work. So I just quickly threw together a draft PR, thinking that it might be useful to Jason (and perhaps also logically went w/ the PR I did last year).

I think Jason has actually abandoned the rule he was working on. That doesn't necessarily mean this isn't still a worthwhile chang to consider though, but I'll leave that to the TS team to decide. If you think getUnknownType should have been exposed in this original PR (#52473), then I can do the work to finish out this change as well.

If you think there's no need to expose it, we can just abandon the effort.

@jakebailey
Copy link
Member

I suspect that if it had been included, we would have accepted it given we already have getAnyType exported.

But, since I sent and merged #56448 which exposed isTypeAssignableTo, maybe it's useful. Then again, anything is assignable to unknown, so maybe that's not the intended use. Hence, my curiosity in how you are able to use it.

@sandersn
Copy link
Member

sandersn commented Apr 1, 2025

We haven't had time to finish reviewing this in over a year.
Since the Corsa API will be quite different from the Strada one, does it make sense merge this? I think it's better to close it and revisit what types we expose as we work on the Corsa API.

@jakebailey
Copy link
Member

We'll likely want this both before and after. I'm still surprised we didn't have this.

@sandersn
Copy link
Member

sandersn commented Apr 1, 2025

I'm not opposed to merging it here. @sstchur do you want to mark this as Ready for review?

@sstchur
Copy link
Contributor Author

sstchur commented Apr 1, 2025

Sure thing!

@sstchur sstchur marked this pull request as ready for review April 1, 2025 23:46
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@sstchur
Copy link
Contributor Author

sstchur commented Apr 1, 2025

It's been so long here, that I don't recall if there is something more I need to do? Do I need to add tests, or otherwise update this PR?

@jakebailey
Copy link
Member

You will need to update tests, yes. Should just be npm test and npx hereby baseline-accept and commit what changed. The PR is old enough that GitHub won't run CI on it, unfortunately.

@jakebailey
Copy link
Member

I foolishly forgot that I already approved and merged #60502, which does this. So, closing as done, oops.

@jakebailey jakebailey closed this Apr 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants