Repository navigation
chore: lint changes - #119
Conversation
- Add the require-future-block-capture rule - Replace legacy ESLint config with flat config - Upgrade ESLint plugins and TypeScript ESLint tooling - Apply lint fixes for the new rules
BundleMonNo change in files bundle size Unchanged groups (1)
Final result: ✅ View report in BundleMon website ➡️ |
| const ancestors = sourceCode.getAncestors(node); | ||
| for (let idx = ancestors.length - 1; idx >= 0; idx -= 1) { | ||
| const ancestor = ancestors[idx]; | ||
| if (ancestor.type === "ArrowFunctionExpression" || ancestor.type === "FunctionExpression") { |
There was a problem hiding this comment.
False positive: FunctionDeclaration is not in this check, so an await inside a nested async function is attributed to the outer Future.block callback. The same code as an arrow function passes.
Future.block(async $ => {
async function helper() { await fetch(url); } // FunctionDeclaration
return $(Future.fromPromise(helper()));
});
Future.block(async $ => {
const helper = async () => { await fetch(url); }; // ArrowFunctionExpression
return $(Future.fromPromise(helper()));
});yarn lint:
src/lint-demo-1.ts
6:31 error Use `await $(...)` inside `Future.block` local/require-future-block-capture
✖ 1 problem (1 error, 0 warnings)
Suggest adding ancestor.type === "FunctionDeclaration" here.
| if (!isFutureBlockCallback(functionNode)) return; | ||
|
|
||
| const captureParam = getBlockCaptureParam(functionNode); | ||
| if (!captureParam) return; |
There was a problem hiding this comment.
False negative: when the callback has no capture param or a destructured one the rule skips it, so every await in it goes unreported, and in that case no await can be safe.
Future.block(async () => {
await fetch(url); // unsafe, but no diagnostic: no capture param
});
Future.block(async ({ length }) => {
await fetch(url); // unsafe, but no diagnostic: destructured param
return length;
});yarn lint: no output, exit 0.
Suggest reporting every AwaitExpression when captureParam is null (message can fall back to $).
| import requireFutureBlockCapture from "./require-future-block-capture.js"; | ||
|
|
||
| function lint(source: string) { | ||
| const linter = new Linter({ configType: "eslintrc" }); |
There was a problem hiding this comment.
configType: "eslintrc" and linter.defineRule are deprecated in ESLint 9 and removed in 10, so the first bump will make this spec throw at construction time. Against eslint@10.11.0:
TypeError: The 'configType' option value must be 'flat'. The value 'eslintrc' is not supported.
at new Linter (node_modules/eslint/lib/linter/linter.js:790:10)
Flat-config equivalent, mirroring how eslint.config.mjs loads the rule (verified: 3/3 tests pass on 9.39.5, and the same config runs on 10.11.0):
const linter = new Linter();
return linter.verifyAndFix(source, {
languageOptions: { ecmaVersion: 2022, sourceType: "module" },
plugins: { local: { rules: { "require-future-block-capture": requireFutureBlockCapture } } },
rules: { "local/require-future-block-capture": "error" },
});| import noRelativeImportPaths from "eslint-plugin-no-relative-import-paths"; | ||
| import unusedImports from "eslint-plugin-unused-imports"; | ||
|
|
||
| import requireFutureBlockCapture from "./eslint/rules/require-future-block-capture.js"; |
There was a problem hiding this comment.
As this is the first time we do this and we are defining best practices, idea: could we write the rule in TypeScript? .nvmrc pins Node 24 (which has native type stripping), so renaming the rule to .ts and pointing this import at it should just work -> verified with yarn lint, the rule still fires:
src/lint-demo-3.ts
2:27 error Use `await $(...)` inside `Future.block` local/require-future-block-capture
That would let it use ESLintUtils.RuleCreator from @typescript-eslint/utils (already a dependency) instead of untyped context/node, and drop the @ts-expect-error in the spec.
There was a problem hiding this comment.
I think writing the rule in TypeScript wold be better. The only downside is that usually downstream projects do not have node updated to latest LTS versions. In those cases, compiling the rules to CommonJS would be an option, but the tradeoff is that linting would have a build prerequisite.
If we continue to add custom rules (or even a preset of existing rules to enforce coding guidelines), the ideal might be having a separate package like @eyeseetea/eslint-plugin, and inside that package have all custom rules written in TypeScript, but publishing the JavaScript compilation. Then the clients would include something like:
import eyeseetea from "@eyeseetea/eslint-plugin";
export default [
eyeseetea.configs.recommended,
];Please let me know what do you think, if it is better to move it to TypeSript now (and maybe include a build config for exporting to other projects where node does not support TypeScript), or leave it for a follow-up if we do more extensive work in eslint changes.
There was a problem hiding this comment.
I'd keep it simple: Here in the skeleton -> TS. Whenever we want to target a <24 Node -> JS.
|
@MatiasArriola Can we add the related clickup task? I know it's hard for generic/internal tasks like this one. When in doubt, check with the PM. Maybe https://app.clickup.com/t/4528615/869eaf07v ? |
Treat nested function declarations as function boundaries to avoid reporting their await expressions in the enclosing Future.block callback. Report unsafe awaits when the callback has no identifier capture parameter.
Add @typescript-eslint/utils as direct devDependency Add .md doc and reference it from the rule
Avoid repeated code isFutureBlockCall vs isFutureBlockFactoryCall Simplify getFunctionAncestor
There was a problem hiding this comment.
I assume it's ready for a re-review, after you latest changes.
All good as far as I can see!
(The tasks is still not confirmed with the PM, I added the time again here: https://app.clickup.com/t/4528615/869eaf07v)
|
Thanks @tokland!! Nice, I was waiting for a confirmation on a task before asking for another review. Confirmed that's the correct task. Thanks! |
📌 References
📝 Implementation
require-future-block-capture: reports unsafeawaitcalls insideFuture.blockeslint-plugin-react-hooks, include in eslint configNotes:
react-hooks/set-state-in-effect(included in new version of react-hooks) was intentionally disabled to keep backwards compatibility. Should be enabled again after consideration.📹 Screenshots/Screen capture
🔥 Notes to the tester