export TypingsInstaller from tsserverlibrary#53394
Conversation
typescript-bot
added
the
For Uncommitted Bug
zkat
force-pushed
the
dev/kmarchan/export-typings-installer
branch
2 times, most recently
from
07ed201 to
ad45060
Compare
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
1 similar comment
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
| emit(writeFile?: WriteFileCallback, customTransformers?: CustomTransformers): EmitResult | BuildInvalidedProject<T> | undefined; | ||
| } | ||
| type InvalidatedProject<T extends BuilderProgram> = UpdateOutputFileStampsProject | BuildInvalidedProject<T> | UpdateBundleProject<T>; | ||
| namespace JsTyping { |
There was a problem hiding this comment.
Somewhat unfortunate that this is making it out into the non-server API given there's no actual use for it...
There was a problem hiding this comment.
What do you mean? The TypingResolutionHost is something I really would have to implement, iiuc
There was a problem hiding this comment.
Sure, but this is typescript.d.ts, not tsserverlibrary.d.ts, and it's not going to contain the ATA stuff as far as I'm aware (note that the change to this file is type-only).
There was a problem hiding this comment.
I am pretty sure this is unnecessary if we just switch these to direct import instead of leveraging the following re-export from within src/services/_namespaces/ts.ts:
export * from "../../jsTyping/_namespaces/ts";@zkat I think that's a reasonable change to make within this PR.
There was a problem hiding this comment.
@DanielRosenwasser You mean doing import { TypingResolutionHost } from "./jsTyping;"? 'cause that broke the tests because it can't find it?? (sorry, I'm not entirely understanding how the namespace mapping thing works right now)
There was a problem hiding this comment.
Yeah, our bundling is unfortunately far too restrictive to allow this sort of thing right now. I think it's just going to have to be gross.
There was a problem hiding this comment.
Hold up, why can't the tests just do a direct import as well?
There was a problem hiding this comment.
@zkat if you change existing instances of TypingResolutionHost to direct imports in the test suite, does the bundler still "break"?
There was a problem hiding this comment.
Okay, I just realized that we (just below here) already have ts.server declared in typescript.d.ts. This isn't a regression; you can see the same thing in TS 4.9.5 too.
So, I think I'm okay with this PR as-is. It's no worse than our current API in terms of over-declaring a public API. ATA already leaks into typescript.d.ts as it is so I don't think there's really much to do here.
zkat
force-pushed
the
dev/kmarchan/export-typings-installer
branch
from
ad45060 to
c998d7a
Compare
| versionMajorMinor, | ||
| } from "./_namespaces/ts"; | ||
|
|
||
| /** @internal */ |
There was a problem hiding this comment.
Instead of removing internal can we re-export it in server as some other name so that typescript.d.ts doesnt change.
There was a problem hiding this comment.
This may not actually work under the naive dts bundler, actually, but we'd have to try.
jakebailey
left a comment
jakebailey
left a comment
There was a problem hiding this comment.
Per my last comment, I think that this is fine, even though it over-declares stuff into the non-server API file.
It turns out that we already over-declare in typescript.d.ts and had been doing that from the start, all thanks to jsTyping. If we had been using regular imports rather than namespace merging, we would have split this code apart a lot differently than what it has ended up as now.
I think that fixing that needs to be a separate API change not in this PR.
|
@sheetalkamat @DanielRosenwasser Are you alright with this PR as-is, given the above? |
5 participants
Fixes #53414
This exports interfaces necessary for external implementers of ITypingsInstaller and extenders of TypingsInstaller.