Skip to content

Conversation

@chris-martin
Copy link
Contributor

@chris-martin chris-martin commented Oct 24, 2025

I'm not sure I understand the problem I'm trying to fix, but it sounds like the gist is that the files we're trying to restyle are interfering with the installation of the tool that does the restyling. My hope is that installation of prettier as a Nix package will be more isolated. Since the markdown restyler doesn't need a lot of the stuff that you may need to restyle JS files, I've implemented this separately rather than have it extend the prettier restyler. I think a similar change could be made, though, to the other Prettier-based restylers; we could put together e.g. a "prettier-with-tailwind" nix package, etc.

@chris-martin chris-martin force-pushed the prettier-nix branch 2 times, most recently from 2c0929d to 6156cce Compare October 24, 2025 22:51
@chris-martin chris-martin marked this pull request as ready for review October 24, 2025 22:56
@pbrisbin
Copy link
Member

pbrisbin commented Oct 27, 2025

I think the approach has merit. In fact, I wouldn't be against totally re-implementing everything on nix; it would just be a pretty big lift at this point.

To get this specific fix out, I think there's three things:

  1. I'd like to keep things inheriting from the prettier base if at all possible, just to reduce maintenance

    I think it should work to make the fix there, as long as nix can also install our blessed plugins. It's fine for them to be installed in the images used for json/yaml/markdown as long as the installation method doesn't cause problems there, like it was/is now.

  2. Whatever we do, Renovate should still be keeping things up to date automatically

    If we specified package-versions in a separate flake file (or whatever, I have no idea how this works), will Renovate automatically do it? Otherwise, we'll need more custom managers.

  3. I want to reproduce all known failures before we put in any fixes

    There is already a test case in this area under prettier, so you could see how to do it (tl;dr: support.path).

    We need to reproduce "Failed to link yarn modules" if project uses a vendored yarn of newer version #928, Prettier fails when using pnpm as package manager #1106, and the (not yet reported) issue you're hitting in prettier-markdown.

Are these things you'd be interested in tackling, or should I take it from here and come back when/if I trip over nix specifically?

@chris-martin
Copy link
Contributor Author

as long as nix can also install our blessed plugins

I think so, I'll see.

If we specified package-versions in a separate flake file (or whatever, I have no idea how this works)

I think you would make a directory that just contains flake.nix and flake.lock, it exports whatever package ("derivation") e.g. prettier, and the Dockerfile can copy in that directory and run nix profile install <the directory>.

will Renovate automatically do it?

https://docs.renovatebot.com/modules/manager/nix/ -

Nix functionality is currently in beta testing, so you must opt-in to test it.

I'll turn this on in the Freckle flakes repo and see how it works.

I want to reproduce all known failures

Would appreciate help with that.

the (not yet reported) issue you're hitting in prettier-markdown

I haven't even figured out how to reproduce that outside of megarepo CI.

@pbrisbin
Copy link
Member

I haven't even figured out how to reproduce that outside of megarepo CI.

Me either!

This runs the exact command in the exact restyler image being used on CI. And it works fine.

% docker run --rm -it --volume $(pwd):/code public.ecr.aws/restyled-io/restyler-prettier:v3.6.2-3 /app/node_modules/.bin/prettier --write frontend/educator/docs/README.md
frontend/educator/docs/README.md 36ms (unchanged)

Only the image, command, and directory contents could affect behavior, and everything looks exactly the same to me (I ran in latest megarepo@main).

So I'm pretty baffled as well.

@chris-martin
Copy link
Contributor Author

chris-martin commented Oct 27, 2025

Is the idea that the prettier restyler should always sort imports, or that it should only do it if it's working in a package that wants that plugin?

(edit) Oh overmind I see, it's controlled by .prettierrc.

@pbrisbin
Copy link
Member

Yup, Restyled is not opinionated, so it should always follow what the project has configured.

@chris-martin
Copy link
Contributor Author

How does that work? Is it doing that presently? I don't see any code in this repo that reads from .prettierrc.

@pbrisbin
Copy link
Member

I don't see any code in this repo that reads from .prettierrc.

It does it by not doing anything. It runs a tool in a directory that is expected to hold configuration and that tool is expected to use that configuration. In this case prettier should be reading .prettierrc.

@chris-martin chris-martin marked this pull request as draft October 27, 2025 19:07
@chris-martin
Copy link
Contributor Author

I have sort of a local bug reproduction and fix. I don't think Nix actually has anything to do with the solution. You just have to make sure your execution of prettier takes place in working directory that contains node_modules with all the needed plugins installed. Fortunately, this is fine because Prettier does not need to run from a directory that contains the source code being linted. I can e.g. do

# Move to a directory that contains the right node_modules
cd /nix/store/nw387q5idifi4d7yskiia3q0n36i6ir7-prettier-current/lib/node_modules/@restyled/restyler-prettier

# Restyle a file elsewhere
/nix/store/nw387q5idifi4d7yskiia3q0n36i6ir7-prettier-current/bin/prettier \
  /home/chris/code/megarepo-frontend/frontend/educator/entities/ts/math/adaptive/models/math-adaptive-domains-practiced.ts

And prettier is able to load plugins from node_modules in the working directory, and it will also do the right thing and walk up the directory tree from file being restyled (not from the working directory) to discover the appropriate /home/chris/code/megarepo-frontend/frontend/educator/.prettierrc file.

So the non-Nix build could actually be modified to do this, but I think first I'll try the approach of making the Nix package output a wrapper script that just changes directory before running.

@pbrisbin
Copy link
Member

Yes, I hit on a similar solution in branch I have locally. What I did was just clobber ./node_modules with ours, run, then restore it after.

I guess this situation could be made simpler, and just add (then run, and remove) our node_modules only if one does not already exist (because the project doesn't commit it).

@pbrisbin
Copy link
Member

pbrisbin commented Oct 27, 2025

Pushed my local branch as #1112 🤷

@chris-martin chris-martin force-pushed the prettier-nix branch 9 times, most recently from d189e76 to f2034a4 Compare October 27, 2025 22:17
@chris-martin chris-martin marked this pull request as ready for review October 27, 2025 22:17
@chris-martin
Copy link
Contributor Author

chris-martin commented Oct 27, 2025

Okay, I think I've got this into a good state.

Nixpkgs offers a very straightforward buildNodeModules function; its inputs are a directory with package.json and package-lock.json and the Node distribution to use; its output is a directory containing node_modules with all the dependencies installed in it. From there it's easy enough to build a wrapper script that changes to that directory and executes prettier. The only somewhat obnoxious part is having to resolve any relative path arguments to absolute paths first so they can still resolve after changing directory.

The JS and Nix dependencies are all in standard package/lock files that I would expect any automated tool to be able to update. Enabled Nix integration for Renovate.

I'm not convinced that the additional test cases are really necessary with this approach. Since the wrapper script does not run prettier from the project directory, it is fairly "obvious" that the project's package.json et al cannot interfere with it.

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.

2 participants