Skip to content

Conversation

AugustinMauroy
Copy link
Member

Description

Add utility to handle changes for package.json

Related issues

Related to #94

@AugustinMauroy AugustinMauroy requested a review from Copilot August 29, 2025 22:54
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new utility module for handling package.json modifications using AST manipulation. It provides functions to locate scripts sections, find Node.js usage within scripts, and replace Node.js arguments.

Key changes:

  • New package-json.ts utility with functions for AST-based package.json manipulation
  • Comprehensive test suite covering all utility functions
  • Added @ast-grep/lang-json dependency for JSON parsing support

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.

File Description
utils/src/ast-grep/package-json.ts Core utility functions for package.json AST manipulation
utils/src/ast-grep/package-json.test.ts Test suite covering all utility functions with edge cases
utils/package.json Added @ast-grep/lang-json dependency for JSON parsing

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@AugustinMauroy AugustinMauroy requested a review from a team August 30, 2025 07:43
AugustinMauroy and others added 2 commits August 30, 2025 14:37
@AugustinMauroy AugustinMauroy marked this pull request as draft August 31, 2025 14:00
@AugustinMauroy AugustinMauroy marked this pull request as ready for review August 31, 2025 14:26
Comment on lines +68 to +91
export const replaceNodeJsArgs = (packageJsonRootNode: SgRoot, argsToValues: Record<string, string>, edits: Edit[]) => {
for (const usage of getNodeJsUsage(packageJsonRootNode)) {
const text = usage.text();
const bashAST = astGrep.parse("bash", text).root();
const command = bashAST.findAll({ rule: { kind: "command" } });
for (const cmd of command) {
const args = cmd.findAll({
rule: {
kind: "word",
not: {
inside: { kind: "command_name" },
},
},
});
for (const arg of args) {
const oldArg = arg.text();
const newValue = argsToValues[oldArg];
if (newValue) {
edits.push(arg.replace(newValue));
}
}
}
}
};
Copy link
Member

Choose a reason for hiding this comment

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

I think just returning the edits is a better API than passing in a variable to be mutated in this context.

Because function caller can have full control about what to do with edits.

Suggested change
export const replaceNodeJsArgs = (packageJsonRootNode: SgRoot, argsToValues: Record<string, string>, edits: Edit[]) => {
for (const usage of getNodeJsUsage(packageJsonRootNode)) {
const text = usage.text();
const bashAST = astGrep.parse("bash", text).root();
const command = bashAST.findAll({ rule: { kind: "command" } });
for (const cmd of command) {
const args = cmd.findAll({
rule: {
kind: "word",
not: {
inside: { kind: "command_name" },
},
},
});
for (const arg of args) {
const oldArg = arg.text();
const newValue = argsToValues[oldArg];
if (newValue) {
edits.push(arg.replace(newValue));
}
}
}
}
};
export const replaceNodeJsArgs = (packageJsonRootNode: SgRoot, argsToValues: Record<string, string>) => {
const edits: Edit[] = []
for (const usage of getNodeJsUsage(packageJsonRootNode)) {
const text = usage.text();
const bashAST = astGrep.parse("bash", text).root();
const command = bashAST.findAll({ rule: { kind: "command" } });
for (const cmd of command) {
const args = cmd.findAll({
rule: {
kind: "word",
not: {
inside: { kind: "command_name" },
},
},
});
for (const arg of args) {
const oldArg = arg.text();
const newValue = argsToValues[oldArg];
if (newValue) {
edits.push(arg.replace(newValue));
}
}
}
}
return edits
};

Copy link
Member Author

Choose a reason for hiding this comment

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

currently api will be use like that:

import { removeNodeJsArgs } from "@nodejs/codemod-utils/ast-grep/package-json";
import type { SgRoot, Edit } from "@codemod.com/jssg-types/main";

/**
 * vX-to-vY
 */
export default function transform(root: SgRoot): string | null {
	const rootNode = root.root();
	const edits: Edit[] = [];

	replaceNodeJsArgs(root, { "--experimental-feature": "--feature"}, edits);	
	removeNodeJsArgs(root, ["--foo"], edits);
	if(!edits.length) return null;


	return rootNode.commitEdits(edits);
}

With your proposal:

import { removeNodeJsArgs, replaceNodeJsArgs } from "@nodejs/codemod-utils/ast-grep/package-json";
import type { SgRoot, Edit } from "@codemod.com/jssg-types/main";

/**
 * vX-to-vY
 */
export default function transform(root: SgRoot): string | null {
	const rootNode = root.root();
	const edits: Edit[] = [];

	edits.push(
		replaceNodeJsArgs(rootNode, { "--experimental-feature": "--feature"})
	);
	edits.push(
		removeNodeJsArgs(rootNode, ["--foo"])
	);
	if(!edits.length) return null;


	return rootNode.commitEdits(edits);
}

So In overal I didn't have any strong opinion but when you are looking at code format/style my aproach is simpler but your is more descriptive to what happened

export const removeNodeJsArgs = (
packageJsonRootNode: SgRoot,
argsToRemove: string[],
edits: Edit[]
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Suggested change
edits: Edit[]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants