fix(valid-repository-directory): use repository root for more accurate linting

[altano]

PR Checklist

• Fixes 🐛 Bug: valid-repository-directory rule always fails #252
• That issue was marked as status: accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

Commits

There are two commits:

1. "settings" - these are just ignores I'm adding to keep my local development environment out of this project (e.g. I use devbox to manage my tool chain, so I'm ignoring those files from the repo)
2. "fix ..." - the actual lint rule fixes

@altano/repository-tools Dependency

The code changes here aren't that big because we're leaning on my @altano/repository-tools library to do the hardest part: discovering the root of the repository.

Notes about this package:

• SCMs supported: git / mercurial / sapling / subversion

• Performance: the library can find a git repository's root in ~3ms, and the worst (slowest) repository type (Mercurial) takes ~148ms. I think this is totally acceptable for package.json linting:

  operation	scm	hz	min	max	mean	p75	p99	p995	p999	rme
  findRootSync	git	294.36	3.0661	5.4844	3.3972	3.4789	4.3417	5.4844	5.4844	±1.37%
  findRootSync	mercurial	6.7348	146.39	152.29	148.48	149.20	152.29	152.29	152.29	±0.91%
  findRootSync	sapling	28.1276	33.7934	37.6630	35.5523	36.0934	37.6630	37.6630	37.6630	±1.55%
  findRootSync	subversion	69.9382	13.3270	18.7447	14.2983	14.2685	18.7447	18.7447	18.7447	±2.08%

• This repository is well tested. As part of the CI flow it has unit tests that create real repositories for each supported type and fully exercise the code.

If you don't want to depend on this library, we can pull in the very small amount of code directly into this repo, but you'd be losing out on the really good CI testing.

Rule Fix

When in a supported repository, the logic for the lint rule is simple:

• Get the repository root path
• We calculate the relative path from the repository root to the directory of the package.json file being linted
• The package.json "directory" field should be that path
• If it isn't, we suggest it
• Example:
  • repository root: "~/src/project"
  • package.json file being linted: "~/src/project/packages/packageA/package.json"
  • the expected value for "directory" in package.json is "packages/packageA"

When a repository root cannot be found, there is a fallback:

• We make sure the "directory" value appears at the end of the package.json directory path. Since we don't know the root of the repository, we can't tell how much of the path has to be present. This can be inaccurate. For example, if package.json is at '/a/b/c/package.json':
  • We make sure "directory" is either "/a/b/c" or "b/c" or "c" (these all validate)
  • If you specify a directory that doesn't appear in this path, e.g. 'd', validation fails
  • If you specify something from the middle of the path, e.g. just 'b', validation fails (we could relax the rule and allow this to pass, but I can't think of a situation in which this would be valid)
• There are no suggestions in this fallback mode.

[altano]

@JoshuaKGoldberg I left this PR in "draft" mode because I wanted to ask if you knew how to test this as an actual ESLint rule? There are the unit tests but I would love to test this in a real repo if possible. Any clue? Or can we publish a beta or something to npm and I'll test it that way? Once it's better tested I can publish.

[altano]

@JoshuaKGoldberg OH, shoot:

The tests are failing because I published @altano/repository-tools as an ESM-only package, and this lint rule has a CJS build which fails to run b/c of the error: Error [ERR_REQUIRE_ESM]: require() of ES Module ...

We have options to proceed here:

1. I re-publish @altano/repository-tools as a dual CJS / ESM module, which I would really, really love to avoid doing.
2. Node.js added support for requiring ESM modules, but it's behind an experimental flag even in 22.7.0: https://nodejs.org/api/modules.html#loading-ecmascript-modules-using-require, so I assume that's a non-starter (for now)
3. This package could be published ESM-only
4. Something else?

Is 1 the only viable option? Thoughts?

[JoshuaKGoldberg]

Yeah I think we're stuck with 1 😞. This package's version support range includes Node versions older than the require-sync-ESM-in-CJS bridge. And a lot of the ESLint ecosystem is still running on CJS.

Tangentially related, a thought: @altano/repository-tools is a pretty wide package name. I see it only has findRoot(Sync) in it now but the name implies it'll have more added later. I'd rather not take a dependency on a large multi-purpose package just to use 1-2 of its functions. What do you think about publishing a dedicated package with a name like @altano/find-root? That way too, if it has to have CJS/ESM dual emit, your other packages don't need that?

[altano]

Alright, yeah, I don't see any other way forward, so I'll publish a dual package.

w.r.t. package surface area: I was planning on adding more utility functions to the "repository-tools" package, but only ones that are thin wrappers around the CLI commands. For example, the findRootSync method will (in a git repo) just run git rev-parse --quiet --show-toplevel to get the result (with equivalents for other SCM types). So it's very light on business logic and will forever remain so. But I'm happy to publish separate, smaller packages and then an umbrella one, so it's totally your call and I'm happy to oblige. Just LMK!

[altano]

@JoshuaKGoldberg I investigated the best way to break a monolithic package up. Packages like lodash seem to have gone through multiple methods (including having separate packages for each method) and landed on the approach of having one package, but allowing subpath exports into the individual files (this is documented here). Then you get the best of all worlds: it's tree-shaking friendly, but shared code won't be duplicated across sub-packages.

So, in addition to supporting ESM/CJS, I'm adding sub-path exports so you can do this:

import { findRootSync } from "@altano/repository-tools/findRootSync";

I also added unit tests to actually exercise the sub-path exports and make sure they work.

Does this all sound good to you? To reiterate: I'm down for whatever, so feel free to suggest an alternative.

[JoshuaKGoldberg]

Cool, that sounds good to me!

I'm not super picky right now. Even if we somehow get it horribly wrong now, as you said, the package is pretty small. We can always tweak it later. 🙂

[JoshuaKGoldberg]

OK! This is phenomenal, awesome job @altano! I tried this out in a few repositories locally. I couldn't break it at all. Nice! 👏

I just have little nitpicky questions that don't really change much. I can go ahead and apply them in a couple days.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.88%. Comparing base (7aaf31d) to head (e480176).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #498      +/-   ##
==========================================
+ Coverage   98.79%   98.88%   +0.08%     
==========================================
  Files          17       17              
  Lines         995     1074      +79     
  Branches       93      100       +7     
==========================================
+ Hits          983     1062      +79     
  Misses         12       12              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[altano]

The compliance action failed with this message:

Warning: PR Body did not match required format
Error: Resource not accessible by integration

🤷🏽‍♀️

[altano]

Thanks for the review. All comments resolved or replied to. Back in your court!

[JoshuaKGoldberg]

OK! Thanks for all your work on this @altano, it's looking great!

[github-actions]

🎉 This is included in version v0.15.3 🎉

The release is available on:

• GitHub releases
• npm package (@latest dist-tag)

Cheers! 📦🚀