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

Fix apheleia-npx in Yarn PnP projects #301

Merged
merged 20 commits into from
Sep 3, 2024

Conversation

zeorin
Copy link
Contributor

@zeorin zeorin commented May 1, 2024

  • apheleia-npx would use an incorrect path for the Yarn PnP ESM loader.
  • apheleia-npx did not correctly guard against word splitting.
  • apheleia-npx was sometimes not able to find formatters in a Yarn PnP project if there was also a node_modules folder at the root of the project. Unfortunately, many tools (including Prettier) will create a cache folder in node_modules even in Yarn PnP projects. The presence of any node_modules folders are irrelevant when a .pnp.cjs file is present.

@zeorin zeorin force-pushed the fix/apheleia-npx branch 2 times, most recently from 70846f5 to d37fbd2 Compare May 1, 2024 23:04
@zeorin
Copy link
Contributor Author

zeorin commented May 1, 2024

cc @edslocomb & @raxod502, as you were involved in #200.

@raxod502
Copy link
Member

LGTM generally. Looks like upstream changes broke the tests. I'll fix, and also make sure we have test coverage that actually validates this change.

@zeorin zeorin force-pushed the fix/apheleia-npx branch from d08f233 to de9c393 Compare June 26, 2024 13:58
edslocomb
edslocomb previously approved these changes Aug 26, 2024
Copy link
Contributor

@edslocomb edslocomb left a comment

Choose a reason for hiding this comment

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

The path/filename bugfixes are great, it took me a while to figure out how the thing worked at all without them ;)

I had initially coded a preference for node_modules over pnp.js so as not to surprise anyone with changed behavior, but it does make a lot more sense to use pnp if present

Apologies to @zeorin for taking so long to look at this, notifications got lost in the shuffle

@raxod502
Copy link
Member

raxod502 commented Sep 1, 2024

Apologies to @zeorin for taking so long to look at this, notifications got lost in the shuffle

Also my fault because I was in the middle of working on this and it was still in progress but I have been busy with other things in my life and not attending to any of my GitHub notifications for a while, there are several dozen threads that have been waiting. This is still very much on my list if Ed doesn't beat me to it.

@raxod502
Copy link
Member

raxod502 commented Sep 3, 2024

Heyo, it turns out that I had got this pretty close to working already! The latest few changes seem to make things fully successful and now we have a proper test framework that supports ensuring that features like this will not break in the future.

@raxod502 raxod502 merged commit f149268 into radian-software:main Sep 3, 2024
4 checks passed
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