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

Export API to provide lib.d.ts listing/mapping #54011

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

@jakebailey
Copy link
Member Author

Checked, and this variable has been defined (with type Map) since 3.0, which predates TS in the playground, so this is a really good option for a backwards compatible playground and so on.

@orta
Copy link
Contributor

orta commented Apr 25, 2023

Nice, thanks! Will update the vfs side to use the private implementation and can have it prefer whatever implementation ends up happening here

@fatcerberus
Copy link

This leads to people duplicating the list externally

you mean like this?
https://github.com/fatcerberus/ts-sphere/blob/master/cell/ts-tool.js#L71-L87

also having to do all this is kind of annoying too
https://github.com/fatcerberus/ts-sphere/blob/master/cell/ts-tool.js#L102-L116

@jakebailey
Copy link
Member Author

Er, why not call ts.getDefaultLibFileName for the former? Not sure what's going on in the second example, sorry.

@fatcerberus
Copy link

fatcerberus commented Apr 25, 2023

@jakebailey I assumed that would end up just calling my custom callback here.

Second example is getting a directory listing while handling includes, excludes, ignoring node_modules, etc. In particular if I don’t do that last thing then it ends up pulling in .js and .ts files from within node_modules as if they were part of my project. I really would like to be able to just give TS an unfiltered directory listing and have it handle all that busywork for me. Especially since tsc uses a slightly different globbing algorithm than what most APIs provide.

@fatcerberus
Copy link

Addendum: The node_modules exclusion is particularly fun because in order to mimic tsc you seem to have to ignore node_modules but not node_modules/@types, which I only figured out by trial and error.

@jakebailey
Copy link
Member Author

That feels really unrelated to this PR.

@fatcerberus
Copy link

It is; I only brought it up as an aside since it’s another case where it’s easy for the logic to fall out of sync with tsc proper. I didn’t mean to imply that should be addressed here, so I apologize.

My concern w.r.t. this PR was my first snippet, but apparently that’s unrelated too (there already being an API that handles that case), so now I’m confused about exactly what this PR does.

@jakebailey
Copy link
Member Author

Per meeting, it seems like this list might not be exhaustive, and that we should just export a function instead of the map itself (which is what I suspected). I'll update the PR with those changes.

In Monaco/VFS we probably will have to still access this or an equivalent for the file list but we could just export and immediately deprecate it, or, just leave it as an unsafe access because those builds aren't going to change.

@jakebailey jakebailey marked this pull request as draft April 26, 2023 21:51
@jakebailey
Copy link
Member Author

Hah, yeah, it's totally not the right thing. There's a major difference between the two lists. Oops.

@jakebailey jakebailey changed the title Export libMap so API users can get lib.d.ts listing Export API to provide lib.d.ts listing/mapping Apr 26, 2023
@jakebailey jakebailey requested a review from sheetalkamat April 26, 2023 23:09
@jakebailey jakebailey marked this pull request as ready for review April 26, 2023 23:09
@jakebailey
Copy link
Member Author

The PR has been overhauled to take a completely different approach. I've added a new generated.ts file that pulls the info out of libs.json like we do diagnosticMessage.json, then added new APIs that use that data.

Happy to take feedback on this!

@orta
Copy link
Contributor

orta commented May 13, 2023

I'd like to throw open the idea that this PR should probably also add some sort of "compiler options to lib list" function?

If you think about how this is would be used in a real-world project, you currently need to pre-download all necessary dts files ( which is what a big chunk of monaco typescript is doing in its build steps ) this PR would allow you to know what those files are in future TS versions.

However, if you wanted to be a bit more elegant, you might not need to download all the necessary files ahead of time, an API letting you know the necessary lib files to predownload would be useful there.

Right now in the playground for example, an ordered array of "all libs" is effectively chopped down from the highest target or lib member, which is OK, but a hack - it'd be nice to be able to ask TypeScript for the exact libs needed

@andrewbranch
Copy link
Member

andrewbranch commented May 15, 2023

I have wanted something similar before, but TypeScript doesn’t actually know what that list is going to be since lib files just reference others. The solution I had in mind when I was thinking about this problem was to make a project with automated builds and publishes that track TypeScript versions (like ts-expose-internals), so non-arbitrary work can be done at build time to determine common bundles. I also wanted to expose literal bundles, not just lists of filenames, both minified and unminified, to further make web-based TS tools easier. I didn’t get around to it because I realized I didn’t need any lib files in the end for arethetypeswrong.github.io, but I still think it would be a nice project. And unfortunately I don’t think TypeScript can provide that as a first-class API.

@andrewbranch
Copy link
Member

I don’t think TypeScript can provide that as a first-class API

Well, I guess what this PR is already doing is build-time processing in service of a first-class API. But the processing would have to get significantly more complicated to accomplish what @orta suggested.

@orta
Copy link
Contributor

orta commented May 15, 2023

That's very reasonable IMO

@jakebailey
Copy link
Member Author

jakebailey commented May 16, 2023

My main reservations about this PR right now are:

  • The added build step and generated file creates a new speedbump in bisecting.
  • I haven't yet figured out what we're going to do across the board to properly get a file list for older packages. This makes me wonder if I should even bother with this API at all... in favor of instead doing something like assuming a CDN with file list support. Or, even just not having this API at all and instead including a JSON file in the package with this info rather than a programmatic interface. After all, the only reason I want this is for VFSs.

@andrewbranch
Copy link
Member

If someone were to implement my idea of doing something like this in a sidecar project, it could build the list for historical versions.

@jakebailey
Copy link
Member Author

If someone were to implement my idea of doing something like this in a sidecar project, it could build the list for historical versions.

I should also say that I want to have this same info available for pack this, but hopefully without any extra build steps past npm publish (since I would like the playground to be just "point at a CDN that hosts the TypeScript package" and it's done), but maybe a service could handle that. But, I also don't really want to actually set up such a project either or make monaco rely on it...

@jakebailey jakebailey marked this pull request as draft May 16, 2023 20:29
@typescript-bot
Copy link
Collaborator

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

* Maps a lib name to its filename in the TypeScript package.
*/
export function getLibFileName(libName: string): string | undefined {
return libMap.get(libName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this lowercase libName eg. ES5 should also get lib.es5.d.ts.. Eg. look at getLibFileNameFromLibReference

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my intent here was that you only ever give this function strings you get from the other APIs, though I just haven't had time to deal with this PR (besides updating it post-dprint) so I can't remember anymore 😅

@sandersn
Copy link
Member

@jakebailey This PR has an approval, but is also still a draft. Is it ready to merge? Does it still need work?

@jakebailey
Copy link
Member Author

No, this will require me to get back into the headspace to work on Monaco and the playground. I plan to come back to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Status: Waiting on author
Development

Successfully merging this pull request may close these issues.

7 participants