fix: declare dedent as an explicit dependency in @herb-tools/linter - #2214
fix: declare dedent as an explicit dependency in @herb-tools/linter#2214jleo3 wants to merge 1 commit into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
luantaraschi
left a comment
There was a problem hiding this comment.
The diagnosis checks out. On main, javascript/packages/linter/package.json lists @herb-tools/*, @ruby/prism, picomatch and tinyglobby, and dedent appears in neither dependencies nor devDependencies, while src/cli/argument-parser.ts:1 and src/rules/erb-no-conditional-html-element.ts:1 both do import dedent from "dedent". The ^1.7.2 you picked also matches what language-server already declares, so the version is consistent with the rest of the repo.
One thing worth knowing before this merges, since it changes how you might want to scope it: the linter is not the only package with this shape. Sweeping every package for a real import dedent from "dedent" under src/ and comparing against its manifest:
| package | import dedent in src/ |
declares dedent |
|---|---|---|
| linter | 2 files | no, this PR |
| formatter | 3 files | no |
| highlighter | 1 file | no |
| language-service | 2 files | no |
| printer | 2 files | no |
| vscode | 1 file | no |
| language-server | none | yes, ^1.7.2 |
So six packages import it undeclared, and the one package that declares it does not import it anywhere in src/. Every one of them would hit the same ERR_MODULE_NOT_FOUND on a direct dist/ import, for the same reason: the bundled entry points inline it and hide the gap.
That is not a reason to hold this PR. Fixing the package the issue was filed against is a fine unit of work, and the version choice is already right. It might be worth a follow-up that declares it in the other five and drops it from language-server, or, if you would rather have one sweep than seven, this PR could grow into it. Happy to send the follow-up myself if you prefer to keep this one as is.
Small note on what I could and could not run: this repo does not check out on Windows, because three fixtures under config/action_view_helpers/ have ? in their filenames, which NTFS rejects. So I read everything through git show and git grep against the object store rather than a working tree, and I did not run the test suite or reproduce the import failure end to end. The manifest and the imports are plain facts of the tree, so I am confident in those; the runtime claim I am taking from your description, which reads correctly against the bundling setup.
|
Correcting my own table above, since I built the follow-up on it and two rows were wrong. I had grepped for
So it is two packages beyond this one, not five. I opened #2231 for those, and it leaves the linter to you here. The part that did hold up is the failure itself: |
|
Thanks for doing the extra research @luantaraschi. Happy to keep the PRs separate and hopes this helps someone else. |
Fixes #2213.
dedentis imported insrc/rules/erb-no-conditional-html-element.tsandsrc/cli/argument-parser.tsbut was missing fromdependenciesinjavascript/packages/linter/package.json. This causesERR_MODULE_NOT_FOUNDfor anyone importing the unbundleddist/modules directly.Change
Add
"dedent": "^1.7.2"todependencies(version already present inyarn.lock).Why it was invisible
bin/herb-lintloads the pre-bundleddist/herb-lint.jswhich already inlinesdedent, so the CLI path is unaffected. The missing declaration only surfaces when importingdist/rules.jsor individual rule modules directly.