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

Automatic Type Acquisition re-write and now available as a separate package #1997

Merged
merged 18 commits into from
Oct 13, 2021

Conversation

orta
Copy link
Contributor

@orta orta commented Aug 24, 2021

ATA works, but there's an awful lot of edge cases. This comes from my original foundations being shaky, but usable. I replicated a bunch of work which TypeScript does to emulate TypeScript running locally, but that's the wrong angle to look at the problem.

This changes ATA to use different conceptual primitives, what ATA used to do was:

  • Did our regexes for import/require/etc catch a module? if so continue
  • Are they absolute? If so, query algolia search API
  • Check Algolia search results for whether types are included or on DT
  • Resolve the 'main' DT import, look inside it for relatives/absolute imports, keep track of the current file for doing relative imports
  • Recurse all of the above

This was made possible because we added enough info to the algolia npm search API so that ATA isn't making a lot of potentially failing requests to figure out what/if the main .d.ts exists. Then we can use unpkg to download each file.

What ATA does now:

  • Use the TypeScript API to get all file references in source code
  • Use the jsdelivr API to get the full file list of the npm module contents for any absolute modules
    • Are there any .d.ts files? If so download them all
    • If there are no .d.ts files? Try download them from @types/[modulename]
  • Recurse

This means the worst case (a module with no types) makes 2 API calls to jsdelivr, any DT request only makes 1 un-useful API call and any lib shipping DT types makes no redundant calls.

The user then puts the files in the right places of their vfs, and the in-memory copy of TypeScript will treat the files no different from on the local FS and that should cover all of the weird edge cases - if it works in your editor, it should work on the playground.


I also added a way to declare a specific npm tag /cc @yyx990803 - right now that is via:

import {xy} from "xyz" // version: beta

but I think I'll probably change the syntax to:

import {xy} from "xyz" // types: beta

so that people don't think ATA is bundling the JS.

@orta orta changed the title Automatic Type Acquisition re-write and separate package Automatic Type Acquisition re-write and now available as a separate package Aug 24, 2021
@orta
Copy link
Contributor Author

orta commented Aug 24, 2021

I wanted to do this to write more tests for this, but instead I ended up watching Downton Abbey oops. There's still time.

@orta
Copy link
Contributor Author

orta commented Sep 24, 2021

Just got to figure out why the sandbox isn't building on CI and this is shippable

@orta orta marked this pull request as ready for review September 24, 2021 10:13
@orta
Copy link
Contributor Author

orta commented Oct 13, 2021

Cool, that works

@orta orta merged commit 980442e into microsoft:v2 Oct 13, 2021
@MartinKolarik
Copy link

Looks like there should be a check for empty values and maybe a debounce added as it currently triggers a fetch before you write the actual package name, e.g. import react from '' => https://data.jsdelivr.com/v1/package/resolve/npm/@latest

@YousefED
Copy link
Contributor

@orta I'm working on migrating a project to this new approach. Something I noticed; for the code import { VegaLite } from "react-vega";, the v2 does 719 requests (to jsdelivr) vs 212 previously (to unpkg).

I suppose this has to do with the fact that now, all .d.ts files are downloaded, instead of recursively resolving local references from the main .d.ts file, correct?

Are we sure this is the right approach? a 3x increase in requests seems quite a heavy cost

@orta
Copy link
Contributor Author

orta commented Oct 21, 2021

Yep, I think it's the right approach - trying to recursively resolve has many drawbacks and cause a lot of 404 requests to get this information - it should be a lot less requests for most libraries and will work with every library. I'm open to improvements though.

@YousefED
Copy link
Contributor

it should be a lot less requests for most libraries and will work with every library. I'm open to improvements though.

Alright, thanks. Making sure this was intended. If we work on improvements perhaps we should add some integration tests to be able to compare metrics like this (404s, total requests, failed requests, etc.) for different libraries.

@orta
Copy link
Contributor Author

orta commented Oct 21, 2021

Sure, you're welcome to look at adding them - so long as they're not flaky.

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

Successfully merging this pull request may close these issues.

4 participants