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

Adding Yarn 2 PnP support #394

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ramblehead
Copy link
Contributor

This patch allows tide to jump to files stored in zip packages (in Yarn 2 cache)
using Emacs arc-mode.

See #388

(This PR replaces closed PR #392)

This patch allows tide to jump to files stored in zip packages (in Yarn 2 cache)
using Emacs arc-mode.

See ananthakumaran#388

(This PR replaces closed PR ananthakumaran#392)
@ananthakumaran
Copy link
Owner

I don't know much about what yarn 2 pnp means, so it would be nice if you can add more info about it in the PR description. Specially want to know if it follows any special file name convention etc.

@ramblehead
Copy link
Contributor Author

ramblehead commented Jul 29, 2020

@ananthakumaran Thank you for the tide mode, and for your reply to this "accidental PR"!

To me tide was (and still is) the first proper environment for JavaScript development (via typescript of-course). And, yarn 2 is the first proper node package management tool - it ticks most of my boxes, including sensible architecture and zero install philosophy on which "PnP" concept is based.

There are many searchable reviews and debates on yarn 2 subject on the Internet. Here I am just assuming that it would be great if at some stage tide could play well with certain yarn 2 peculiarities out-of-the-box :)

To start tide with yarn 2, the following (or similar) emacs config is required:

(setq-local tide-tsserver-executable
            ;; assuming that `project-root', the path to yarn 2 repository, is defined elsewhere
            (concat project-root
                    "/.yarn/sdks/typescript/bin/tsserver"))

a .yarn/sdks bit is described here:
https://yarnpkg.com/advanced/editor-sdks

Yarn 2 eliminates recursive node_modules from its repositories. Instead, it is using flat .yarn/cache which looks like the following:

.yarn/cache$ tree
.
├── abbrev-npm-1.1.1-3659247eab-9f9236a3cc.zip
├── abort-controller-npm-3.0.0-2f3a9a2bcb-cc53ad8df9.zip
├── accepts-npm-1.3.7-0dc9de65aa-2686fa30db.zip
├── acorn-jsx-npm-5.2.0-4c0af33483-1247cc4b32.zip
├── acorn-npm-6.4.1-77905520a8-7aa4623c6d.zip
...
├── next-npm-9.4.5-canary.43-21cd24de92-0fbddd70d8.zip
...

Each external node package (not necessarily from npm) is stored in zip archive. Those archives can be uploaded to SCM (e.g. git) together with the repository source code - thus zero install - just clone and build node repository or the whole monorepository. This node packages structure is called PnP in yarn 2 talk.

The following screenshot shows how tide sees files in such zip packages (notice “next” path in dir tree above and on the screenshot in eldoc):

Screenshot from 2020-07-29 13-53-43

If we strip $$virtual/*/0/ part from what tide gets from tsserver, it becomes a legitimate path within .yarn/cache zip archive. If such zip path is visited in emacs, tramp mode tries to open it - at least that is what happens in my environment. Unfortunately, current tramp archives handling is a bit unstable and has issues (such as files in archives are read-only).

The code proposed in this PR does two things:

  1. strips $$virtual part of file paths in yarn 2 zip packages;
  2. and opens code files (e.g. from tide-jump-to-definition) in those zip packages using arc-mode instead of the default tramp handling.

What I do not understand here is how to correctly treat $$virtual part of zip path (similar to your question on “special file name convention etc.”) and if it is safe to just strip it. So far, stripping worked fine with my yarn 2 experimental projects. Probably, need to ask this question to Yarn 2 community...

@ramblehead
Copy link
Contributor Author

ramblehead commented Jul 29, 2020

I found what is the purpose of $virtual in yarn 2:
https://yarnpkg.com/advanced/pnpapi#resolvevirtual
https://yarnpkg.com/advanced/lexicon#virtual-package

and in yarn 2 code:
https://github.com/yarnpkg/berry/blob/5f0439a087b8463d97fb70c96dc6f983a91be512/packages/yarnpkg-fslib/sources/VirtualFS.ts#L14

Basically, the virtual part of the package path is used in yarn 2 to track peerDependencies. To get the real package file path it is ok to strip the virtual part - that is what this PR does.

@jkolyer
Copy link

jkolyer commented Oct 17, 2020

I too am a fan of both tide and yarn2. Would like to see support in tide. If there's anything I can do to help, test, debug, whatever, LMK.

@rsimoes
Copy link

rsimoes commented Dec 8, 2020

Does this have a chance of getting merged in? It'd be nice to have Yarn 2 support without having to maintain one's own local fork.

@josteink
Copy link
Collaborator

josteink commented Dec 8, 2020

I'm not opposed to adding yarn-support to tide, but for the maintainers to merge this I think we would need at least the following 3 things:

  • all conflicts against git master to be resolved
  • clear instructions on how this is supposed to be used (maybe a purpose-built demo-repo using yarn can be used as a demo?), written for someone who has never before used yarn.
  • clear instructions on how to test the provided changes (again, maybe with a purpose-built demo-repo), written for someone who has never before used yarn.

Basically the current form of this PR puts the burden of all those things onto the maintainers, instead of the contributor, and that's often the number one reason for closed PRs.

So anything which lowers the effort required to "OK" this is probably going to help this get merged.

@ramblehead
Copy link
Contributor Author

As it turns out, $$virtual/*/0/ part cannot be just striped out from the yarn 2 virtual path:
yarnpkg/berry#499 (comment)

The algorithm explained by arcanis at the link above is quite simple - we just need to implement it in elisp. Otherwise, I have been using this patch via advice-add since summer in a mid-size multi-package yarn 2 project and quite happy with it:

https://github.com/ramblehead/.emacs.d/blob/c585bd2cb73ced0985b10834779825cbecdb12d2/lisp/config-tide.el#L66

The major limitation of this patch is that once inside a yarn 2 package archive, tide does not work there and if one needs to navigate/refactor/etc. within packages she would need to unplug them first. I do not know, even conceptually, how to make tide (or lsp) work from within archives...

@josteink thank you for the points outlining contribution requirements - I will try to find a bit of time to follow them...

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.

None yet

5 participants