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

Add support for formatters locally installed via yarn 2+ pnp mode #200

Merged
merged 24 commits into from
Nov 5, 2023

Conversation

edslocomb
Copy link
Contributor

This adds support for formatters installed locally in project directories via yarn 2's "zero install" pnp mode.

It's quite similar to the support for formatters installed locally in a project's node_modules via npm, and leverages the npx symbol, so existing formatter definitions should work without modification.

This checks for a .pnp.cjs file (expected in the project root for yarn pnp projects), then looks for a yarn executable, and checks the version of yarn to make sure it supports pnp. If that works, we just push "yarn" onto the front of command.

I've only tested this with a locally installed prettier.js. It's very much a works-for-me draft, I'm putting in a PR to make sure this is a workable approach before going any further with it.

@zarybnicky
Copy link

Trying this out, I see that the yarn --version invocation takes more than half a second - which, compared to the actual prettier runtime, takes inexcusably long. (Testing with Yarn 4rc45 and Yarn 3.2.0 both.)

In my experience, the existence of .pnp.cjs marks the current project as a Yarn 2+ project pretty much without exception and the version check can be skipped - is your experience different?

@edslocomb
Copy link
Contributor Author

I added the version check because I ran into a problem with emacs invoking an outdated system version of yarn instead of the project local version, but I'm fine with leaving it to the developer to make sure they've got their external toolchain in order :)

apheleia-core.el Outdated Show resolved Hide resolved
apheleia-core.el Outdated Show resolved Hide resolved
@raxod502
Copy link
Member

Sorry it took so long, but take a look at the latest commits I pushed to your branch. This introduces a new approach which provides full PNP compatibility while adding essentially no overhead (PNP-installed Prettier runs in 148ms with my version, versus 285ms when running via Yarn, versus 115ms when invoking the binary directly).

@raxod502
Copy link
Member

And CI is broken, because of packaging bullshit. I guess I'll have to fix that too, will do either today or later...

@edslocomb
Copy link
Contributor Author

Excellent! I was noodling around with a special case of this that would call node directly instead of through yarn, but only for prettier. The more general approach here is very nice.

I've also pondered spinning up a node process with prettier loaded, and then calling the api to do the formatting, thus sparing the user the overhead of loading node every time... but that's a project for another day ;)

@edslocomb
Copy link
Contributor Author

Currently working on getting this running in a project with a .pnp.cjs and .pnp.loader.mjs, not a .pnp.js.

@raxod502
Copy link
Member

Filed prettier/plugin-ruby#1392 upstream about the remaining CI error.

@edslocomb
Copy link
Contributor Author

Getting back to this, with a few notes:

  • This won't work in Windows emacs, not sure if there would be affected users
  • No idea if it works with tramp mode
  • Was puzzled by apheleia-npx looking for a .pnp.js rather than a .pnp.cjs, changed it but should be easy to make it work with both

@edslocomb
Copy link
Contributor Author

edslocomb commented Oct 10, 2023

I needed to add minor fixes to apheleia-ft.el to get CI working; will extract those into a separate PR if the yarn-pnp stuff needs more work after review

@raxod502
Copy link
Member

Hmm, dunno about .js versus .cjs, when I ran yarn pnp locally it generated .pnp.js so maybe this is a version thing.

Copy link
Member

Choose a reason for hiding this comment

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

Was it intended to include these files?

Copy link
Contributor Author

@edslocomb edslocomb Oct 13, 2023

Choose a reason for hiding this comment

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

Yes, those zip files are where the dependencies live

...or did you just mean the @types/emscripten dependency, specifically? I haven't tried removing that

Copy link
Member

@raxod502 raxod502 left a comment

Choose a reason for hiding this comment

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

Makes sense to me to write this in JavaScript if it is more robust, however having dependencies is not so nice - what leads to the requirement to find formatters in zip files? When would that be usable?

@raxod502
Copy link
Member

raxod502 commented Oct 13, 2023

Also, prettier/plugin-ruby#1392 was closed, so applying the suggested fix from there here. I see you did that already, thanks.

@edslocomb
Copy link
Contributor Author

Yarn's pnp mode caches package zip files and reads them as needed instead of caching the unzipped files.

A package can suggest that it should be unplugged (unzipped) via the "preferUnplugged": true in package.json (prettier does this), but it is not not required for packages that export binaries/executables (e.g. eslint does not).

@edslocomb
Copy link
Contributor Author

edslocomb commented Oct 13, 2023

Hmm, dunno about .js versus .cjs, when I ran yarn pnp locally it generated .pnp.js so maybe this is a version thing.

What does yarn --version say? If I can repo the behavior I think making this work with a .pnp.js as well will be pretty straightforward

@edslocomb
Copy link
Contributor Author

I've extracted and cleaned up the pnp-bin js script; it's set up to build a minified standalone nodejs executable.

The minified standalone is ~290k and targets node 8 or higher. Would it be better to use that here?

@raxod502
Copy link
Member

Okay, yeah, that makes sense to me. If we at least have a standalone script, it's still not terribly nice, but at least the vendoring is restricted to a plaintext file. Thank you for doing this! I don't have any other major concerns about the PR.

* When searching for an executable in node_modules, iterate
through node's list of possible node_modules paths, provided by
require.resolve.paths('')

* Decide whether to use pnp or npm resolution based on the closest
parent directory containing a .pnp.cjs file or node_modules
directory, with a tie going to npm
@edslocomb
Copy link
Contributor Author

Commit ef82e42 should fix #217

Copy link
Member

@raxod502 raxod502 left a comment

Choose a reason for hiding this comment

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

@raxod502
Copy link
Member

raxod502 commented Nov 4, 2023

You think we should merge this for now and then iterate from there? There are getting to be a fair number of issues fixed here but not on main. (I've given you admin permissions on the repository so you can merge things yourself)

@edslocomb
Copy link
Contributor Author

edslocomb commented Nov 5, 2023

Sounds good to me, there's definitely been some creep in this PR (I don't see a PR merge button on my end btw)

@edslocomb edslocomb merged commit 54a192c into radian-software:main Nov 5, 2023
4 checks passed
alternateved added a commit to alternateved/apheleia that referenced this pull request Nov 6, 2023
ItsHoff added a commit to ItsHoff/Dotfiles that referenced this pull request Feb 5, 2024
radian-software/apheleia#200
seems to have broken prettier formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants