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

JS/TS package acquisition #182801

Closed
wants to merge 10 commits into from
Closed

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented May 17, 2023

This PR packages @zkat's work to enable package acquisition on the web. This is broken into 3 parts:

  1. A replacement for npm that runs in the browser. Right now it's orogene -- for our purposes, a drop-in npm replacement. But it could be something much simpler and hackier if we want to avoid shipping a large WASM bundle at first. See packageManager.ts; the associated README has instructions on how to install orogene; I didn't include it in this PR yet although Kat intends for it to be vendored eventually.
  2. Support for installing packages from package.json: This PR adds a second and third virtual filesystems for node_modules that, when accessed, use the npm replacement to install packages and cache them. The extension's tsserver web host changes slightly to use the third filesystem as a fallback for paths containing node_modules. This should avoid errors on imports in TS files, but will also help both TS and JS intellisense.
  3. Support for installing packages via ATA: This PR adds an implementation of typescript's ATA installer host. This is only needed for JS projects, although those with package.json will mostly use (2) above. This change depends on 5.1, because the installer host interface was not public until then.

The only thing I've done to zkat's work is to merge from main and fix the extension's tsconfigs so they can correctly load WASM bundles without having to switch to ES modules. (thanks @andrewbranch for helping me with this)

As far as I can tell, the current status of this code is: working end-to-end for a demo scenario, with a variety of bugs blocking other scenarios. For example, after I got the code to build and run, when I tried to load a project from a local disk, I got errors about missing files in the virtual filesystem. My guess is that most of the bugs are minor and that there are two or three major ones lurking.

The design could probably use some work as well, but I believe it's good enough to check in and start iterating on.

Here are the remaining TODOs from the prototype PR:

  • Figure out why ATA doesn't kick in unless I close a .ts file first???
  • Figure out why Go To Source Definition for non-module targets isn't working
  • Add localStorage (or such?) support for caching previously-loaded typings
  • Figure out story with CORS, esp for private registries.
  • Fix file watching to make sure stuff gets updated on package.json edits (Note: will probably need to work with Nathan on this. idk what's going on with watching in general)
  • Case sensitivity control?

@mjbvz
Copy link
Collaborator

mjbvz commented May 18, 2023

Thank you for the PR. Just testing it out. A few blockers on merging this even in an experimental state:

  • @mjbvz The build is currently broken. This is cause by the main TS extension referencing ../../package-manager in a few places, which causes the compiled output of the TS extension to be shifted one layer down (it ends up in out/src/* instead of the expected out/*)

    I may just stick the one package-manager file under src for now.

  • @sandersn I don't see node-maintainer in this PR? Was this meant to be included? I believe we need to check it in for now (alternatively, we can publish it to npm and pick up the package)

  • @mjbvz We need a new experimental setting to disable this feature by default so that we don't break users on web

@sandersn
Copy link
Member Author

I did not check in node-maintainer, but Kat did intend for it to be checked in.

@kieferrm kieferrm mentioned this pull request May 22, 2023
76 tasks
This includes the executable files node_maintainer.js and
node_maintainer_bg.wasm. I had to skip linting on this commit because
node_maintainer.js does not follow vscode standards.
@mjbvz
Copy link
Collaborator

mjbvz commented Jun 6, 2023

I've copied this PR over to #184438. This should unblock the yarn.lock check

@mjbvz mjbvz closed this Jun 6, 2023
@kieferrm kieferrm mentioned this pull request Jun 11, 2023
74 tasks
@kieferrm kieferrm mentioned this pull request Jul 9, 2023
54 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Jul 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants