Repository navigation
chore: lint changes #119
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| function isIdentifier(node, name) { | ||
| return node?.type === "Identifier" && node.name === name; | ||
| } | ||
|
|
||
| function isFutureBlockCall(node) { | ||
| return ( | ||
| node?.type === "CallExpression" && | ||
| node.callee.type === "MemberExpression" && | ||
| !node.callee.computed && | ||
| isIdentifier(node.callee.object, "Future") && | ||
| isIdentifier(node.callee.property, "block") | ||
| ); | ||
| } | ||
|
|
||
| function isFutureBlockFactoryCall(node) { | ||
| return ( | ||
| node?.type === "CallExpression" && | ||
| node.callee.type === "MemberExpression" && | ||
| !node.callee.computed && | ||
| isIdentifier(node.callee.object, "Future") && | ||
| isIdentifier(node.callee.property, "block_") | ||
| ); | ||
| } | ||
|
|
||
| function getBlockCaptureParam(functionNode) { | ||
| const [firstParam] = functionNode.params; | ||
| return firstParam?.type === "Identifier" ? firstParam : null; | ||
| } | ||
|
|
||
| function getFunctionAncestor(sourceCode, node) { | ||
| 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") { | ||
|
Contributor
There was a problem hiding this comment. False positive: 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()));
});
Suggest adding |
||
| return ancestor; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| function isFutureBlockCallback(functionNode) { | ||
| const parent = functionNode.parent; | ||
| if (parent?.type !== "CallExpression") return false; | ||
|
|
||
| if (isFutureBlockCall(parent) && parent.arguments[0] === functionNode) { | ||
| return true; | ||
| } | ||
|
|
||
| if (parent.arguments[0] !== functionNode) return false; | ||
|
|
||
| const callee = parent.callee; | ||
| if (callee.type !== "CallExpression") return false; | ||
|
|
||
| return isFutureBlockFactoryCall(callee); | ||
| } | ||
|
|
||
| module.exports = { | ||
| meta: { | ||
| type: "problem", | ||
| docs: { | ||
| description: | ||
| "Require await calls inside Future.block callbacks to go through the capture function", | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| wrapAwait: "Use `await {{capture}}(...)` inside `Future.block`.", | ||
| }, | ||
| }, | ||
| create(context) { | ||
| const sourceCode = context.sourceCode; | ||
|
|
||
| return { | ||
| AwaitExpression(node) { | ||
| const functionNode = getFunctionAncestor(sourceCode, node); | ||
| if (!functionNode) return; | ||
| if (!isFutureBlockCallback(functionNode)) return; | ||
|
|
||
| const captureParam = getBlockCaptureParam(functionNode); | ||
| if (!captureParam) return; | ||
|
Contributor
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 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;
});
Suggest reporting every |
||
|
|
||
| const awaited = node.argument; | ||
| if ( | ||
| awaited.type === "CallExpression" && | ||
| awaited.callee.type === "Identifier" && | ||
| awaited.callee.name === captureParam.name | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| context.report({ | ||
| node, | ||
| messageId: "wrapAwait", | ||
| data: { capture: captureParam.name }, | ||
| }); | ||
| }, | ||
| }; | ||
| }, | ||
| }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import { Linter } from "eslint"; | ||
| import { describe, expect, test } from "vitest"; | ||
|
|
||
| // @ts-expect-error Local CommonJS rule has no TypeScript declaration. | ||
| import requireFutureBlockCapture from "./require-future-block-capture.js"; | ||
|
|
||
| function lint(source: string) { | ||
| const linter = new Linter({ configType: "eslintrc" }); | ||
|
Contributor
There was a problem hiding this comment.
Flat-config equivalent, mirroring how 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" },
}); |
||
| linter.defineRule("require-future-block-capture", requireFutureBlockCapture); | ||
|
|
||
| return linter.verifyAndFix(source, { | ||
| parserOptions: { ecmaVersion: 2022, sourceType: "module" }, | ||
| rules: { "require-future-block-capture": "error" }, | ||
| }); | ||
| } | ||
|
|
||
| describe("require-future-block-capture", () => { | ||
| test("allows Futures captured in Future.block", () => { | ||
| const result = lint("Future.block(async $ => await $(Future.success(1))); "); | ||
|
|
||
| expect(result.messages).toEqual([]); | ||
| }); | ||
|
|
||
| test("allows Futures captured in Future.block_", () => { | ||
| const result = lint("Future.block_()(async capture => await capture(Future.success(1))); "); | ||
|
|
||
| expect(result.messages).toEqual([]); | ||
| }); | ||
|
|
||
| test("reports but does not rewrite a native Promise", () => { | ||
| const source = "Future.block(async $ => await Promise.resolve(1));"; | ||
| const result = lint(source); | ||
|
|
||
| expect(result.output).toBe(source); | ||
| expect(result.messages).toMatchObject([{ messageId: "wrapAwait" }]); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,6 @@ | |
| "$/*": ["./*"] | ||
| } | ||
| }, | ||
| "include": ["src", "eslint/**/*.ts"], | ||
| "references": [{ "path": "./tsconfig.node.json" }] | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is the first time we do this and we are defining best practices, idea: could we write the rule in TypeScript?
.nvmrcpins Node 24 (which has native type stripping), so renaming the rule to.tsand pointing this import at it should just work -> verified withyarn lint, the rule still fires:That would let it use
ESLintUtils.RuleCreatorfrom@typescript-eslint/utils(already a dependency) instead of untypedcontext/node, and drop the@ts-expect-errorin the spec.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep it simple: Here in the skeleton -> TS. Whenever we want to target a <24 Node -> JS.