Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This comment has been minimized.
This comment has been minimized.
packages/site/src/content/docs/rules/browser/nodeRemoveMethods.mdx
Outdated
Show resolved
Hide resolved
e801097 to
9d2a8da
Compare
lishaduck
left a comment
There was a problem hiding this comment.
Looks much better, but I'd still ask that this not use any. If we're not declaring the actual types, twoslash isn't really helping us ensure our examples are correct (and it looks less cool 🙃).
packages/site/src/content/docs/blog/what-flint-does-differently.mdx
Outdated
Show resolved
Hide resolved
packages/site/src/content/docs/rules/browser/nodeRemoveMethods.mdx
Outdated
Show resolved
Hide resolved
packages/site/src/content/docs/rules/flint/invalidCodeLines.mdx
Outdated
Show resolved
Hide resolved
packages/site/src/content/docs/rules/jsx/ariaHiddenFocusables.mdx
Outdated
Show resolved
Hide resolved
| <Component isActive={true} /> | ||
| declare const Component: any; | ||
| // ---cut--- | ||
| <Component isActive={true} />; |
There was a problem hiding this comment.
The semicolon is annoying me... we can't just add a .prettierrc override for .mdx & semicolons though, that'd mess up the actual ts. Hmmm.
|
@lishaduck, I think I got all the ones we can meaningfully define. I left |
lishaduck
left a comment
There was a problem hiding this comment.
Ok, this does look much better, but there's certainly still some anys hanging out.
I got through about the first maybe quarter of the files, hopefully that's enough to show what I mean?
packages/site/src/content/docs/blog/what-flint-does-differently.mdx
Outdated
Show resolved
Hide resolved
packages/site/src/content/docs/rules/flint/invalidCodeLines.mdx
Outdated
Show resolved
Hide resolved
| declare const RuleContext: any; | ||
| declare const _: any; | ||
| declare const ruleCreator: any; |
There was a problem hiding this comment.
We can definitely get better types here.
packages/site/src/content/docs/rules/flint/nodePropertyInChecks.mdx
Outdated
Show resolved
Hide resolved
packages/site/src/content/docs/rules/flint/nodePropertyInChecks.mdx
Outdated
Show resolved
Hide resolved
packages/site/src/content/docs/rules/jsx/unnecessaryFragments.mdx
Outdated
Show resolved
Hide resolved
packages/site/src/content/docs/rules/node/blobReadingMethods.mdx
Outdated
Show resolved
Hide resolved
packages/site/src/content/docs/rules/node/filePathsFromImportMeta.mdx
Outdated
Show resolved
Hide resolved
packages/site/src/content/docs/rules/node/fileReadJSONBuffers.mdx
Outdated
Show resolved
Hide resolved
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
This is looking great! +1 to lishaduck's review. My only additional area of requested changes now is to liberally use // @noErrors to avoid adding in unnecessary syntax to what readers see.
Since this PR touches a lot, you're getting a lot of feedback. If you'd prefer to just touch some of the files and leave other areas as followups, that could work too! Whatever's easier for you 🙂.
Thanks for getting started on this - I'm really excited to have the added intellisense-y hovers on the site!
packages/site/src/content/docs/blog/what-flint-does-differently.mdx
Outdated
Show resolved
Hide resolved
|
|
||
| ```tsx | ||
| // @errors: 2322 -- string "true"/"false" isn't assignable to boolean prop | ||
| <button autoFocus="false" /> |
There was a problem hiding this comment.
autoFocus="false" triggers TS2322 here because the string "false" isn't assignable to a boolean prop — and that type error is pointing at a real problem. In JSX, autoFocus="false" passes a truthy string, so the browser will still autofocus this element.
isSetToFalse in the rule implementation treats the string "false" as equivalent to boolean false, which looks like a bug in the rule itself. I'll change this example to I deleted this example since we already had that exact example included. Should I file a separate issue for the rule? And should we also remove the technically correct but misleading autoFocus={false}"true" example?
|
@JoshuaKGoldberg, in the course of adding TypeScript rule suppressions, it made me wonder if those Flint rules are redundant with TypeScript. In those cases, it's not possible to trigger the Flint rule without also incurring a TypeScript violation. Given our philosophy of Tooling Coordination, it seems we could simply defer to showing the TypeScript error rather than checking for the same violation on the linting side. |
9e2c091 to
2aa94dc
Compare
Co-authored-by: Josh Goldberg ✨ <github@joshuakgoldberg.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2aa94dc to
b418dac
Compare

PR Checklist
status: accepting prsOverview
twoslashglobally rather than on per codeblocktwoslashand determine how to handle it--cut--to hide them from view while providing contextScreenshot(s)
Appendix
Here is the temp script I used