Eng 369 style guide automation #241
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update migrates ESLint configurations across multiple project directories to ESLint's flat config format, replacing legacy Changes
Sequence Diagram(s)sequenceDiagram
participant VSCode
participant Developer
participant ESLint
participant TypeScript
Developer->>VSCode: Opens workspace
VSCode->>ESLint: Applies workspace settings (flat config, workingDirs)
Developer->>ESLint: Runs "lint" script
ESLint->>TypeScript: Reads tsconfig(.lint).json for project info
ESLint->>ESLint: Loads flat config (eslint.config.mjs/js)
ESLint->>Developer: Reports lint results
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
apps/roam/eslint.config.mjs (1)
6-13: Sameproject: trueissue as in apps/websiteThe boolean will crash the parser; replace it with the path to the lint tsconfig.
packages/ui/eslint.config.js (1)
6-13:project: trueis invalid here as wellProvide the explicit tsconfig path to keep ESLint functional.
- tsconfigRootDir: ".", - project: true, + tsconfigRootDir: ".", + project: "./tsconfig.lint.json",apps/obsidian/eslint.config.mjs (1)
6-13:projectneeds a path, nottrueSame correction as the other configs—point to
./tsconfig.lint.json.
🧹 Nitpick comments (5)
apps/website/package.json (1)
11-12: Consider adding an auto-fix variant of the lint script.A second script such as
"lint:fix": "eslint . --fix"is cheap to wire up and dramatically improves DX by letting developers apply safe autofixes in one step.No action required if your workflow already covers this elsewhere.
discourse-graph.code-workspace (1)
22-25: Broaden watcher exclusions to reduce VS Code churn.You already ignore
target; most projects in this repo generatedistand/or.next. Excluding them here prevents unnecessary file-watcher load."files.watcherExclude": { "**/target": true, + "**/dist": true, + "**/.next": true },packages/ui/tsconfig.lint.json (1)
1-3: AddnoEmitto prevent accidental JS output during lintingEven though the file is only used by ESLint, TypeScript may still attempt to emit JS if
noEmitis not explicitly disabled upstream. Being explicit removes that risk and signals intent.{ - "extends": "./tsconfig.json" + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true + } }packages/eslint-config/next.js (2)
13-15: Consider setting minimum Node.js version to 20.11.As the comment suggests, you could use
import.meta.dirnamedirectly if you set the minimum Node.js version to 20.11. This would simplify the code and remove the workaround.
40-47: Important: Document the parserOptions limitation.The comment indicates that
parserOptionsmust be repeated in each project becausecwdfails in editors. This is a significant limitation that should be:
- Documented in the project's ESLint setup guide
- Verified in all consuming projects
#!/bin/bash # Description: Verify that consuming projects have proper parserOptions configuration echo "=== Checking ESLint configs in apps for parserOptions ===" fd -e mjs -e js "eslint.config" apps/ --exec grep -l "parserOptions" {} \; echo -e "\n=== Checking for tsconfig.lint.json files ===" fd "tsconfig.lint.json" apps/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/roam/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.vscode/settings.json(1 hunks)apps/obsidian/eslint.config.mjs(1 hunks)apps/obsidian/package.json(1 hunks)apps/obsidian/tsconfig.json(1 hunks)apps/obsidian/tsconfig.lint.json(1 hunks)apps/roam/eslint.config.mjs(1 hunks)apps/roam/package.json(1 hunks)apps/roam/tsconfig.json(1 hunks)apps/roam/tsconfig.lint.json(1 hunks)apps/website/.eslintrc.js(0 hunks)apps/website/eslint.config.mjs(1 hunks)apps/website/package.json(1 hunks)apps/website/tsconfig.json(2 hunks)apps/website/tsconfig.lint.json(1 hunks)discourse-graph.code-workspace(1 hunks)package.json(2 hunks)packages/database/tsconfig.json(1 hunks)packages/database/tsconfig.lint.json(1 hunks)packages/eslint-config/library.js(0 hunks)packages/eslint-config/next.js(1 hunks)packages/eslint-config/package.json(1 hunks)packages/eslint-config/react-internal.js(1 hunks)packages/ui/.eslintrc.js(0 hunks)packages/ui/eslint.config.js(1 hunks)packages/ui/package.json(2 hunks)packages/ui/tsconfig.json(1 hunks)packages/ui/tsconfig.lint.json(1 hunks)
💤 Files with no reviewable changes (3)
- apps/website/.eslintrc.js
- packages/eslint-config/library.js
- packages/ui/.eslintrc.js
🧰 Additional context used
🪛 Biome (1.9.4)
apps/website/tsconfig.lint.json
[error] 2-3: Expected a property but instead found '}'.
Expected a property here.
(parse)
🔇 Additional comments (19)
apps/obsidian/tsconfig.lint.json (1)
1-3: Config is minimal and correctExtending the project’s main
tsconfig.jsonwithout overrides is the recommended pattern for ESLint’sparserOptions.project.apps/roam/tsconfig.lint.json (1)
1-3: Consistent lint-specific tsconfig – looks goodMatches the approach adopted in other packages; no issues spotted.
packages/database/tsconfig.lint.json (1)
1-3: OK as a pass-through lint configNo problems; inherits all options from the primary
tsconfig.json.packages/ui/package.json (2)
19-19: Raising--max-warningsto 20 dilutes lint gate – verify intentPreviously the script failed on any warning; the new threshold allows up to 20 warnings before CI breaks.
Confirm this relaxation is intentional and that CI still enforces the style-guide standards you expect.
33-34: Eslint patch upgrade – 👍Updating to
8.57.1is safe; no breaking changes between patch releases.apps/roam/tsconfig.json (1)
3-3:distexclusion is spot-onExcluding the emitted
distfolder prevents the compiler from needlessly scanning build artefacts and eliminates false-positive duplicate identifier errors in project references.apps/roam/package.json (1)
10-10: Locallintscript added – confirm Turbo picks it upWith Turborepo, a package-level
"lint"script is automatically executed byturbo run lint.
Double-check that the rootturbo.json(orpackage.jsonpipeline) still targets the workspace’slinttask so the command isn’t orphaned.package.json (1)
13-15: Prettier 3 upgrade – run the formatter onceJumping from
3.4.x→3.6.xcan update heuristics and whitespace rules.
Runnpm run format(or equivalent) and commit any large formatting diff now to avoid polluting future PRs.apps/website/tsconfig.json (2)
4-4:baseUrlset to project rootGood call – this simplifies absolute imports (
~/foo) and aligns with the existingpathsmap.
22-22: Excludedistto avoid stale type leakageKeeps the compiler from ingesting old
.d.tsfiles emitted by Next.js builds.apps/obsidian/tsconfig.json (1)
17-17: Addsdistto the ignore listMatches the pattern applied across sibling apps and prevents type clashes with bundled output.
packages/ui/tsconfig.json (1)
3-4: LGTM – simplification is consistent with the new shared config.No functional or tooling regressions detected.
.vscode/settings.json (1)
5-7: Settings align with ESLint flat-config migration.Verified that VS Code picks up these keys; nothing further to change.
apps/obsidian/package.json (2)
10-10: LGTM! Standard lint script addition.Adding the ESLint script aligns with the PR's goal of style guide automation.
18-18: ```shell
#!/bin/bashVerify if the obsidian app imports any Node.js built-in modules
echo "=== Checking for ES import of Node.js built-ins ==="
rg -n "import .* from '"['"]" apps/obsidianecho -e "\n=== Checking for require() of Node.js built-ins ==="
rg -n "require('"['"])" apps/obsidian</details> <details> <summary>packages/eslint-config/package.json (2)</summary> `6-6`: **Good! ES module declaration aligns with flat config migration.** The `"type": "module"` declaration is necessary for the ES module exports used in the ESLint flat config files. --- `13-19`: [web_search] ```web What breaking changes were introduced in eslint-config-prettier v10.1.5 compared to v9.x?packages/eslint-config/react-internal.js (2)
29-29: Clarify the commented turbo config.The turbo config is commented out here but included in
next.js. Is this intentional? Please either:
- Remove the comment if not needed
- Add a comment explaining why it's excluded for React internal libraries
26-82: Good consistency with next.js config structure.The configuration maintains excellent consistency with the
next.jsconfig, which will make maintenance easier. The main differences are appropriate for the different use cases (Next.js apps vs React internal libraries).
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
from
ef83e10 to
8da3b0a
Compare
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
from
8da3b0a to
be41004
Compare
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
from
be41004 to
e79b3b7
Compare
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
from
e79b3b7 to
d23bb68
Compare
|
This is now working and tested. I updated quite a few dependencies, and I advise everyone to have a look. But it also gives us much more comprehensive linting in vscode and cursor. The variable name setting is noisy and needs tuning, but even then I think it's worth incorporating. |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
There is a lot going on here and there are huge ramifications for these decisions. I'm not sure where to begin. Some initial thoughts:
Let separate this into multiple smaller PR's, so we can go through them in order. If that is not possible, could you add inline comments in GitHub's UI to help explain what changes are made and what problem they are solving.
EG: Could each of the following be it's own PR? If not, for each, could you link to the files changed, or specific commit, and explain the choices made.
I decided to start with a modernization of our eslint configuration, with new flat configs.
Then, this did not work as well in vscode, so I had to be more more explicit about subprojects.
Finally I added some new functionality: checking for variable names (may need tuning), arrow functions and parameter count. (The issue with no-used-vars got solved by the new config not relying on eslint:recommended, but the typescript version.)
Formatting
There is a bunch of formatting changes in this PR as well.
Could this be it's own PR to help reduce the noise?
package-lock.json
Roam's package-lock.json was removed. Is this necessary in this PR? Why?
Also, have you checked and does this solve the reason it was still there?
|
Each step is a separate commit, and the third could be made into its own pr easily. It makes less sense to separate the other two: basically after the first, the cli works well but vscode cannot parse anything. I'll see about separating format later tonight, but I doubt you'll gain that much. The sad fact is that this was going from one non working state to a working state with days of trial and error; none of the steps worked until the end; and finding functional seams may be more days of work. What I propose instead: let's look at it (minus last step) as a whole together, tell me what you're uncomfortable about, and we'll see if we can revert it without breaking. |
|
Because there are so many files changed and many of the commits are force-pushed (thus unnamed), it would be extremely helpful if you could provide some additional context. Even if it is just linking a commit to each step listed above. |
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
from
d23bb68 to
7c9ffd7
Compare
|
Ok. I separated the last layer from the previous one. I'll try to break it down even more, but this is a step. |
|
One other comment: The endless force-push that I realize you're not comfortable with are how I was separating the layers, so whatever was new was separate from just fixing/rewriting the old. I had to reorder commits to maintain this. This allowed me to separate these steps easily just now. (ETA: missing a negation) |
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
from
7c9ffd7 to
b04a008
Compare
maparent
force-pushed
the
ENG-521-eslint-flat-config
branch
from
6eb3a2e to
d990d4d
Compare
|
Ok, on roam/package-lock.json: That was a mistake on my part, I got into situations where I would add a package, npm install, run in trouble, revert my package.json change, reinstall, and the trouble would remain there, i.e. there was path-dependency and reverting changes to package.json did not get me back to status quo. So I zapped node_modules and package-lock a few times to get to a pristine state and retry. It was a nightmare. Still, between removing something and git rm, there is a huge difference, and I should have noticed. That said: without the package-lock, I was building fine. So it's possible the issue you describe has gone away through other updates. (maybe including the ones on this branch.) Not sure how to test this, we should look at it together. |
|
Ok, it's getting late, so here's what I see: First, let's be clear: I can delineate logical units, but none of the steps will be runnable until the last.
Breaking this into separate commits is probably ~3h work. |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Can next import react-internal so we can de-duplicate code?
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
from
fa076a9 to
4badb54
Compare
maparent
force-pushed
the
ENG-521-eslint-flat-config
branch
from
d162479 to
9061e69
Compare
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
4 times, most recently
from
f391e59 to
4541bce
Compare
maparent
force-pushed
the
ENG-521-eslint-flat-config
branch
from
76aafa8 to
e7f1471
Compare
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
from
4541bce to
a3089bc
Compare
maparent
force-pushed
the
ENG-521-eslint-flat-config
branch
from
e7f1471 to
f627479
Compare
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
from
a3089bc to
439256f
Compare
maparent
force-pushed
the
ENG-521-eslint-flat-config
branch
2 times, most recently
from
ca51ac2 to
ab6b589
Compare
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
2 times, most recently
from
bfb9c7e to
5e6345a
Compare
maparent
force-pushed
the
ENG-521-eslint-flat-config
branch
from
ab6b589 to
cf46de8
Compare
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
from
5e6345a to
613be9b
Compare
maparent
force-pushed
the
ENG-521-eslint-flat-config
branch
from
cf46de8 to
bbb4a41
Compare
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
from
613be9b to
2f11733
Compare
maparent
force-pushed
the
ENG-521-eslint-flat-config
branch
from
bbb4a41 to
0384559
Compare
maparent
force-pushed
the
ENG-369-style-guide-automation
branch
from
2f11733 to
50b334e
Compare
maparent
force-pushed
the
ENG-521-eslint-flat-config
branch
from
0384559 to
baa2946
Compare
* ENG-369: eslint with new flat configuration files. * make it work in vscode * coderabbit improvements * make it look closer to turbo skeleton * most additions have no obvious effect * add preferArrows, naming conventions, max-params. (#241) * Update tsconfig.json to include trailing comma in compilerOptions --------- Co-authored-by: Michael Gartner <mclicks@gmail.com>
Those are the new changes to the eslint configuration:
prefer-arrow-functions,
naming-conventions,
max-params.