feat(net-setsimultaneousaccepts-migration): create recipe #278

vespa7 · 2025-11-19T13:14:23Z

Closes #173

AugustinMauroy · 2025-11-19T21:18:33Z

package.json

    "migrations",
-    "node.js"
-  ],
+    "node.js"  ],


you should revert this change

recipes/net-setSimultaneousAccepts-migration/README.md

AugustinMauroy

Missing test case for dynamic import

vespa7 · 2025-11-19T22:32:26Z

Missing test case for dynamic import

Yes, it’s in draft. It doesn’t need to be corrected yet. I’m waiting for a question to be resolved.

JakobJingleheimer · 2025-11-20T09:02:53Z

Yes, it’s in draft. It doesn’t need to be corrected yet. I’m waiting for a question to be resolved.

I don't see a question. Is it something we can help with? (Could you point us to it?)

AugustinMauroy · 2025-11-28T08:25:58Z

package.json

    "codemod",
    "migrations",
-    "node.js"
+    "node.js"  


Suggested change

"node.js"

"node.js"

AugustinMauroy · 2025-11-28T08:26:49Z

What the current state of this pr ?

vespa7 · 2025-11-28T10:56:14Z

What the current state of this pr ?

The PR is ready to be reviewed.

AugustinMauroy

not too bad, good job but there are serval part of the implementation that need to be discussed

AugustinMauroy · 2025-11-29T17:42:54Z

recipes/net-setSimultaneousAccepts-migration/codemod.yaml

@@ -0,0 +1,21 @@
+schema_version: "1.0"
+name: "@nodejs/net-setSimultaneousAccepts-migration"


Suggested change

name: "@nodejs/net-setSimultaneousAccepts-migration"

name: "@nodejs/net-setSimultaneousAccepts-deprecation"

#namingIsHard @nodejs/userland-migrations what do you think

AugustinMauroy · 2025-11-29T17:44:09Z

recipes/net-setSimultaneousAccepts-migration/src/workflow.ts

+	const linesToRemove: Range[] = [];
+
+	const netImportStatements = getAllNetImportStatements(root);
+	if (netImportStatements.length === 0) return null;


Suggested change

if (netImportStatements.length === 0) return null;

// If no import found we don't process the file

if (!netImportStatements.length) return null;

AugustinMauroy · 2025-11-29T17:44:59Z

recipes/net-setSimultaneousAccepts-migration/src/workflow.ts

+		processNetImportStatement(rootNode, statement, linesToRemove, edits);
+	}
+
+	if (edits.length === 0 && linesToRemove.length === 0) return null;


Suggested change

if (edits.length === 0 && linesToRemove.length === 0) return null;

// If there aren't any change we don't try to modify something

if (!edits.length && !linesToRemove.length) return null;

Maybe "No changes, nothing to do" for the comment

AugustinMauroy · 2025-11-29T17:46:22Z

recipes/net-setSimultaneousAccepts-migration/src/workflow.ts

+		...getNodeImportStatements(root, 'node:net'),
+		...getNodeImportCalls(root, 'node:net'),
+		...getNodeRequireCalls(root, 'node:net'),


Suggested change

...getNodeImportStatements(root, 'node:net'),

...getNodeImportCalls(root, 'node:net'),

...getNodeRequireCalls(root, 'node:net'),

...getNodeImportStatements(root, 'net'),

...getNodeImportCalls(root, 'net'),

...getNodeRequireCalls(root, 'net'),

the node: isn't needed theses functions catch it

AugustinMauroy · 2025-11-29T17:46:36Z

recipes/net-setSimultaneousAccepts-migration/src/workflow.ts

+	edits: Edit[]
+): void {
+	const bindingPath = resolveBindingPath(statementNode, '$');
+	if (!bindingPath) return;


Suggested change

if (!bindingPath) return;

if (!bindingPath) return;

AugustinMauroy · 2025-11-29T17:47:37Z

recipes/net-setSimultaneousAccepts-migration/src/workflow.ts

+/**
+ * Finds all _setSimultaneousAccepts() call expressions
+ */
+function findSetSimultaneousAcceptsCalls(


This function isn't valuable. You can "hardcode" this query in the parent functio

AugustinMauroy · 2025-11-29T17:48:56Z

recipes/net-setSimultaneousAccepts-migration/src/workflow.ts

+	const objDeclarations = rootNode.findAll({
+		rule: {
+			any: [
+				{ pattern: `const ${objectName} = $_` },


why ast kind/ast query can we have something stronger ?

Mmm, I think there's probably a built-in way from ast-grep to get this without needing such a wide pattern.

If I understand the scenario correctly, it can be rewritten as something like the code below.
It finds all variable declarations that use the name objectName.

const objDeclarations = rootNode.findAll({ kind: 'variable_declarator', has: { kind: 'identifier', field: 'name', patern: objctName, } })

I was thinking in something like this:

const pair = root .root() .findAll({ rule: { kind: "variable_declarator" } }) .filter((decl) => decl.field("name")?.text() === objectName) .flatMap((decl) => decl.findAll({ rule: { kind: "pair" } })) .filter((p) => p.field("key")?.text() === keyName) .map((pair) => pair.field("value")?.text())[0]

@brunocroh @JakobJingleheimer @AugustinMauroy

WTF that completely crazy

@brunocroh yeah, that's exactly the sort of thing I was thinking :)

@vespa7 that is a bit… verbose, and I suspect, overly manual (it looks a bit like a Rube Goldberg machine as code 😄). I think there is a more concise way to do it. I would try something like Bruno suggested.

@AugustinMauroy that's maybe not the most constructive way to phrase it 😅

I think you're misunderstanding what @vespa7's comment is saying due to the thread, but the equivalent to @brunocroh's code in Alex's code is

const pair = rootNode .findAll({ rule: { kind: "variable_declarator" } }) .filter((decl) => decl.field("name")?.text() === objectName)

If Brunos' code works it might be a bit better for that chunk, but the complete piece is substituting

const objDeclarations = rootNode.findAll({ rule: { any: [ { pattern: `const ${objectName} = $_` }, { pattern: `let ${objectName} = $_` }, { pattern: `var ${objectName} = $_` }, ], }, }); for (const objDecl of objDeclarations) { const objectLiterals = objDecl.findAll({ rule: { kind: 'object' } }); for (const obj of objectLiterals) { const pairs = obj.findAll({ rule: { kind: 'pair' } }); for (const pair of pairs) { const key = pair.child(0); if (key?.text() === propertyName) { const rangeWithComma = expandRangeToIncludeTrailingComma( pair.range(), rootNode.text() ); linesToRemove.push(rangeWithComma); } } } }

And if you got that correctly I'm not sure how the new piece looks more verbose. Following on Bruno's proposal for the whole snippet, maybe something like

const pairs = rootNode .findAll({ rule: { kind: "variable_declarator", has: { kind: "identifier", field: "name", pattern: objectName, }, }, }) .flatMap((decl) => decl.findAll({ rule: { kind: "pair", has: { field: "key", regex: keyName, }, }, }), );

You can break it in the middle to give explanatory names to the steps

AugustinMauroy · 2025-11-29T17:49:53Z

recipes/net-setSimultaneousAccepts-migration/src/workflow.ts

+	for (const objDecl of objDeclarations) {
+		const objectLiterals = objDecl.findAll({ rule: { kind: 'object' } });
+
+		for (const obj of objectLiterals) {
+			const pairs = obj.findAll({ rule: { kind: 'pair' } });
+
+			for (const pair of pairs) {
+				const key = pair.child(0);
+				if (key?.text() === propertyName) {
+					const rangeWithComma = expandRangeToIncludeTrailingComma(
+						pair.range(),
+						rootNode.text()
+					);
+					linesToRemove.push(rangeWithComma);
+				}
+			}
+		}
+	}


can we have a loop that push to an array then iterate on this array to avoid 3th level of nested loop

AugustinMauroy · 2025-11-29T17:50:50Z

recipes/net-setSimultaneousAccepts-migration/src/workflow.ts

+	varName: string,
+	linesToRemove: Range[]
+): void {
+	const varDeclarationStatements = rootNode.findAll({


JakobJingleheimer

Thanks for this! It's a good first :)

I think this could use some additional test-cases that include more things happening from node:net to ensure they don't get broken.

JakobJingleheimer · 2025-11-27T22:32:36Z

package.json

    "codemod",
    "migrations",
-    "node.js"
+    "node.js"  


Suggested change

"node.js"

"node.js"

JakobJingleheimer · 2025-11-27T22:38:22Z

recipes/net-setSimultaneousAccepts-migration/README.md

+const net = require("node:net");
+
+-net._setSimultaneousAccepts(false);
+ const server = net.createServer();


Suggested change

const net = require("node:net");

-net._setSimultaneousAccepts(false);

const server = net.createServer();

const net = require("node:net");

- net._setSimultaneousAccepts(false);

const server = net.createServer();

JakobJingleheimer · 2025-11-30T15:28:32Z

recipes/net-setSimultaneousAccepts-migration/src/workflow.ts

+		processNetImportStatement(rootNode, statement, linesToRemove, edits);
+	}
+
+	if (edits.length === 0 && linesToRemove.length === 0) return null;


Maybe "No changes, nothing to do" for the comment

JakobJingleheimer · 2025-12-01T09:37:29Z

recipes/net-setSimultaneousAccepts-migration/src/workflow.ts

+	if (argKind === 'member_expression') {
+		handleMemberExpressionArgument(rootNode, argNode, linesToRemove);
+	} else if (argKind === 'identifier') {
+		handleIdentifierArgument(rootNode, argNode, linesToRemove);
+	}


nit: switch makes it more clear that it's inspecting the same condition for each.

Suggested change

if (argKind === 'member_expression') {

handleMemberExpressionArgument(rootNode, argNode, linesToRemove);

} else if (argKind === 'identifier') {

handleIdentifierArgument(rootNode, argNode, linesToRemove);

}

switch(argKind) {

case 'member_expression': handleMemberExpressionArgument(rootNode, argNode, linesToRemove); break;

case 'identifier': handleIdentifierArgument(rootNode, argNode, linesToRemove); break;

}

JakobJingleheimer · 2025-12-01T09:39:11Z

recipes/net-setSimultaneousAccepts-migration/src/workflow.ts

+		rule: { pattern: `${objectName}.${propertyName}` },
+	});
+
+	// Only remove if this is the only reference


nit: the extra "only" is kind of cumbersome to read

Suggested change

// Only remove if this is the only reference

// Remove when this is the only reference

JakobJingleheimer · 2025-12-01T09:40:59Z

recipes/net-setSimultaneousAccepts-migration/src/workflow.ts

+	const objDeclarations = rootNode.findAll({
+		rule: {
+			any: [
+				{ pattern: `const ${objectName} = $_` },


Mmm, I think there's probably a built-in way from ast-grep to get this without needing such a wide pattern.

feat(net-setsimultaneousaccepts-migration): create recipe

081d5e8

Closes nodejs#173

AugustinMauroy reviewed Nov 19, 2025

View reviewed changes

package.json Outdated

"migrations",

"node.js"

],

"node.js" ],

Copy link

Member

AugustinMauroy Nov 19, 2025

Choose a reason for hiding this comment

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

you should revert this change

AugustinMauroy reviewed Nov 19, 2025

View reviewed changes

recipes/net-setSimultaneousAccepts-migration/README.md Show resolved Hide resolved

AugustinMauroy reviewed Nov 19, 2025

View reviewed changes

Remove variable passed to method

9084315

vespa7 marked this pull request as ready for review November 20, 2025 13:50

JakobJingleheimer added the awaiting reviewer Author has responded and needs action from the reviewer label Nov 27, 2025

AugustinMauroy reviewed Nov 28, 2025

View reviewed changes

package.json

"codemod",

"migrations",

"node.js"

"node.js"

Copy link

Member

AugustinMauroy Nov 28, 2025

Choose a reason for hiding this comment

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

Suggested change

"node.js"

"node.js"

AugustinMauroy reviewed Nov 29, 2025

View reviewed changes

JakobJingleheimer reviewed Dec 1, 2025

View reviewed changes

JakobJingleheimer added awaiting author Reviewer has requested something from the author and removed awaiting reviewer Author has responded and needs action from the reviewer labels Dec 1, 2025

		@@ -0,0 +1,21 @@
		schema_version: "1.0"
		name: "@nodejs/net-setSimultaneousAccepts-migration"

-	if (netImportStatements.length === 0) return null;
+	// If no import found we don't process the file
+	if (!netImportStatements.length) return null;

-	if (edits.length === 0 && linesToRemove.length === 0) return null;
+	// If there aren't any change we don't try to modify something
+	if (!edits.length && !linesToRemove.length) return null;

	// Only remove if this is the only reference
	// Remove when this is the only reference

Uh oh!

feat(net-setsimultaneousaccepts-migration): create recipe #278

Are you sure you want to change the base?

feat(net-setsimultaneousaccepts-migration): create recipe #278

Conversation

vespa7 commented Nov 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AugustinMauroy left a comment

Choose a reason for hiding this comment

Uh oh!

vespa7 commented Nov 19, 2025 • edited by JakobJingleheimer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JakobJingleheimer commented Nov 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AugustinMauroy commented Nov 28, 2025

Uh oh!

vespa7 commented Nov 28, 2025

Uh oh!

AugustinMauroy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brunocroh Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vespa7 Dec 4, 2025 • edited by AugustinMauroy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ceres6 Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vespa7 commented Nov 19, 2025 •

edited by JakobJingleheimer

Loading

brunocroh Dec 4, 2025 •

edited

Loading

vespa7 Dec 4, 2025 •

edited by AugustinMauroy

Loading

Ceres6 Dec 9, 2025 •

edited

Loading

JakobJingleheimer Dec 1, 2025 •

edited

Loading