Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions eslint-factory/eslint.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ module.exports = [
"gh-aw-custom/require-escaped-regexp-interpolation": "warn",
"gh-aw-custom/require-fetch-timeout": "warn",
"gh-aw-custom/require-nan-check-after-env-numeric-parse": "warn",
"gh-aw-custom/prefer-structured-clone": "warn",
},
},
{
Expand Down
2 changes: 2 additions & 0 deletions eslint-factory/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { noDuplicateConstantValuesRule } from "./rules/no-duplicate-constant-val
import { requireEscapedRegexpInterpolationRule } from "./rules/require-escaped-regexp-interpolation";
import { requireFetchTimeoutRule } from "./rules/require-fetch-timeout";
import { requireNanCheckAfterEnvNumericParseRule } from "./rules/require-nan-check-after-env-numeric-parse";
import { preferStructuredCloneRule } from "./rules/prefer-structured-clone";

const plugin = {
meta: {
Expand Down Expand Up @@ -81,6 +82,7 @@ const plugin = {
"require-escaped-regexp-interpolation": requireEscapedRegexpInterpolationRule,
"require-fetch-timeout": requireFetchTimeoutRule,
"require-nan-check-after-env-numeric-parse": requireNanCheckAfterEnvNumericParseRule,
"prefer-structured-clone": preferStructuredCloneRule,
},
};

Expand Down
83 changes: 83 additions & 0 deletions eslint-factory/src/rules/prefer-structured-clone.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import { RuleTester } from "eslint";
import { describe, expect, it } from "vitest";
import { preferStructuredCloneRule } from "./prefer-structured-clone";

const cjsRuleTester = new RuleTester({
languageOptions: {
ecmaVersion: 2022,
sourceType: "commonjs",
},
});

describe("prefer-structured-clone", () => {
it("uses the correct docs URL", () => {
expect(preferStructuredCloneRule.meta.docs.url).toBe("https://github.com/github/gh-aw/tree/main/eslint-factory#prefer-structured-clone");
});

it("valid: unrelated JSON.parse / JSON.stringify usages are accepted", () => {
cjsRuleTester.run("prefer-structured-clone", preferStructuredCloneRule, {
valid: [
`const data = JSON.parse(rawText);`,
`const text = JSON.stringify(obj);`,
`structuredClone(obj);`,
// Replacer/indent argument changes stringify semantics; excluded to avoid false positives.
`const clone = JSON.parse(JSON.stringify(obj, null, 2));`,
// Custom reviver on parse changes semantics too; still matched today since only the
// parse call itself is checked for a single stringify argument — but the inner
// stringify call must have exactly one argument, so this is out of scope here.
Comment on lines +25 to +27
`const clone = JSON.parse(JSON.stringify(obj), reviver);`,
],
invalid: [],
});
});

it("invalid: JSON.parse(JSON.stringify(x)) is flagged and suggests structuredClone(x)", () => {
cjsRuleTester.run("prefer-structured-clone", preferStructuredCloneRule, {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The inline comment on lines 28–30 is misleading: it says "still matched today" but this case is in the valid array (i.e. it is not flagged). The phrase implies the rule fires on this input, which is the opposite of what happens — JSON.parse(JSON.stringify(obj), reviver) is correctly excluded because the outer parse call has 2 arguments.

💡 Suggested clarification
// Custom reviver: excluded because JSON.parse has 2 arguments —
// the node.arguments.length !== 1 guard in the rule rejects it cleanly.
`const clone = JSON.parse(JSON.stringify(obj), reviver);`,

@copilot please address this.

valid: [],
invalid: [
{
code: `const clone = JSON.parse(JSON.stringify(tool));`,
errors: [
{
messageId: "preferStructuredClone",
suggestions: [
{
messageId: "replaceWithStructuredClone",
output: `const clone = structuredClone(tool);`,
},
],
},
],
},
{
code: `const runs = state.runs.map(run => JSON.parse(JSON.stringify(run)));`,
errors: [
{
messageId: "preferStructuredClone",
suggestions: [
{
messageId: "replaceWithStructuredClone",
output: `const runs = state.runs.map(run => structuredClone(run));`,
},
],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is self-contradictory: it claims the reviver case is "still matched today" but the test lists it under valid (i.e., not flagged) — the opposite claim.

💡 Details

The outer JSON.parse(JSON.stringify(obj), reviver) call has two arguments, so node.arguments.length !== 1 triggers an early return in the rule — it is never matched, for a different reason than the comment implies. The comment focuses on the inner stringify call's argument count, but the actual disqualifier here is the outer parse call's second reviver argument. Clarify the comment to state that the parse call itself is excluded due to its own arg count, rather than saying it is "matched" and excluded downstream.

},
],
},
{
code: `const clone = JSON["parse"](JSON["stringify"](tool));`,
errors: [
{
messageId: "preferStructuredClone",
suggestions: [
{
messageId: "replaceWithStructuredClone",
output: `const clone = structuredClone(tool);`,
},
],
},
],
},
],
});
});
});
79 changes: 79 additions & 0 deletions eslint-factory/src/rules/prefer-structured-clone.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import { AST_NODE_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils";

const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`);

/**
* Returns true when the call expression is `JSON.parse(...)` (direct or computed access).
*/
function isJsonParseCall(node: TSESTree.CallExpression): boolean {
const callee = node.callee;
if (callee.type !== AST_NODE_TYPES.MemberExpression) return false;
if (callee.object.type !== AST_NODE_TYPES.Identifier || callee.object.name !== "JSON") return false;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches on the bare identifier name JSON with no scope/binding resolution, so a locally shadowed or aliased JSON will be falsely flagged and auto-fixed to structuredClone(...), silently changing runtime behavior.

💡 Details

isJsonParseCall/isPlainJsonStringifyCall only check callee.object.name === "JSON" textually, with no scope analysis. If any enclosing scope shadows the global JSON (e.g. a parameter, destructure, or test double named JSON), the rule still matches, and because this rule ships an auto-suggest fixer (not just a warning), applying it rewrites to the real global structuredClone, which is not equivalent to the shadowed value — actively breaking code rather than just warning about it.

Fix: resolve JSON via scope (e.g. check context.sourceCode.getScope(node) for a local binding named JSON and bail out) before reporting/fixing.

const property = callee.property;
const isDirectAccess = !callee.computed && property.type === AST_NODE_TYPES.Identifier && property.name === "parse";
const isComputedAccess = callee.computed && property.type === AST_NODE_TYPES.Literal && property.value === "parse";
return isDirectAccess || isComputedAccess;
}

/**
* Returns true when the call expression is `JSON.stringify(...)` (direct or computed access)
* with exactly one argument (a replacer/indent argument changes the round-trip semantics and
* is intentionally excluded from this check to keep false positives low).
*/
function isPlainJsonStringifyCall(node: TSESTree.CallExpression): boolean {
const callee = node.callee;
if (callee.type !== AST_NODE_TYPES.MemberExpression) return false;
if (callee.object.type !== AST_NODE_TYPES.Identifier || callee.object.name !== "JSON") return false;
const property = callee.property;
const isDirectAccess = !callee.computed && property.type === AST_NODE_TYPES.Identifier && property.name === "stringify";
const isComputedAccess = callee.computed && property.type === AST_NODE_TYPES.Literal && property.value === "stringify";
if (!isDirectAccess && !isComputedAccess) return false;
return node.arguments.length === 1;
}

export const preferStructuredCloneRule = createRule({
name: "prefer-structured-clone",
meta: {
type: "suggestion",
hasSuggestions: true,
docs: {
description: "Prefer structuredClone(...) over JSON.parse(JSON.stringify(...)) for deep-cloning plain data in actions/setup/js scripts. The JSON round-trip is slower, silently drops values it cannot represent (undefined, functions, Date becomes a string), and throws on circular references, whereas structuredClone (Node >=17, available globally in the Node 24 runtime this action targets) clones plain objects and JSON-safe data directly.",
},
schema: [],
messages: {
preferStructuredClone: "Replace JSON.parse(JSON.stringify({{arg}})) with structuredClone({{arg}}) — the JSON round-trip silently drops undefined/function values, converts Dates to strings, and throws on circular references.",
replaceWithStructuredClone: "Replace with structuredClone(...).",
},
},
defaultOptions: [],
create(context) {
const sourceCode = context.sourceCode;

return {
CallExpression(node) {
if (!isJsonParseCall(node)) return;
if (node.arguments.length !== 1) return;

const innerArg = node.arguments[0];
if (innerArg.type !== AST_NODE_TYPES.CallExpression) return;
if (!isPlainJsonStringifyCall(innerArg)) return;

const clonedExpressionText = sourceCode.getText(innerArg.arguments[0]);

context.report({
node,
messageId: "preferStructuredClone",
data: { arg: clonedExpressionText },
suggest: [
{
messageId: "replaceWithStructuredClone",
fix(fixer: TSESLint.RuleFixer) {
return fixer.replaceText(node, `structuredClone(${clonedExpressionText})`);
},
},
],
});
},
};
},
});
Loading