feat(DEP0121): net._setSimultaneousAccepts()#278
Conversation
AugustinMauroy
left a comment
There was a problem hiding this comment.
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. |
I don't see a question. Is it something we can help with? (Could you point us to it?) |
| "codemod", | ||
| "migrations", | ||
| "node.js" | ||
| "node.js" |
There was a problem hiding this comment.
| "node.js" | |
| "node.js" |
|
What the current state of this pr ? |
The PR is ready to be reviewed. |
AugustinMauroy
left a comment
There was a problem hiding this comment.
not too bad, good job but there are serval part of the implementation that need to be discussed
| @@ -0,0 +1,21 @@ | |||
| schema_version: "1.0" | |||
| name: "@nodejs/net-setSimultaneousAccepts-migration" | |||
There was a problem hiding this comment.
| name: "@nodejs/net-setSimultaneousAccepts-migration" | |
| name: "@nodejs/net-setSimultaneousAccepts-deprecation" |
#namingIsHard @nodejs/userland-migrations what do you think
There was a problem hiding this comment.
Mm, the suggestion is more correct.
| const linesToRemove: Range[] = []; | ||
|
|
||
| const netImportStatements = getAllNetImportStatements(root); | ||
| if (netImportStatements.length === 0) return null; |
There was a problem hiding this comment.
| if (netImportStatements.length === 0) return null; | |
| // If no import found we don't process the file | |
| if (!netImportStatements.length) return null; |
| processNetImportStatement(rootNode, statement, linesToRemove, edits); | ||
| } | ||
|
|
||
| if (edits.length === 0 && linesToRemove.length === 0) return null; |
There was a problem hiding this comment.
| 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; |
There was a problem hiding this comment.
Maybe "No changes, nothing to do" for the comment
| ...getNodeImportStatements(root, 'node:net'), | ||
| ...getNodeImportCalls(root, 'node:net'), | ||
| ...getNodeRequireCalls(root, 'node:net'), |
There was a problem hiding this comment.
| ...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
| /** | ||
| * Finds all _setSimultaneousAccepts() call expressions | ||
| */ | ||
| function findSetSimultaneousAcceptsCalls( |
There was a problem hiding this comment.
This function isn't valuable. You can "hardcode" this query in the parent functio
| 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); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
can we have a loop that push to an array then iterate on this array to avoid 3th level of nested loop
JakobJingleheimer
left a comment
There was a problem hiding this comment.
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.
| "codemod", | ||
| "migrations", | ||
| "node.js" | ||
| "node.js" |
There was a problem hiding this comment.
| "node.js" | |
| "node.js" |
| const net = require("node:net"); | ||
|
|
||
| -net._setSimultaneousAccepts(false); | ||
| const server = net.createServer(); |
There was a problem hiding this comment.
| const net = require("node:net"); | |
| -net._setSimultaneousAccepts(false); | |
| const server = net.createServer(); | |
| const net = require("node:net"); | |
| - net._setSimultaneousAccepts(false); | |
| const server = net.createServer(); |
| processNetImportStatement(rootNode, statement, linesToRemove, edits); | ||
| } | ||
|
|
||
| if (edits.length === 0 && linesToRemove.length === 0) return null; |
There was a problem hiding this comment.
Maybe "No changes, nothing to do" for the comment
| if (argKind === 'member_expression') { | ||
| handleMemberExpressionArgument(rootNode, argNode, linesToRemove); | ||
| } else if (argKind === 'identifier') { | ||
| handleIdentifierArgument(rootNode, argNode, linesToRemove); | ||
| } |
There was a problem hiding this comment.
nit: switch makes it more clear that it's inspecting the same condition for each.
| 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; | |
| } |
net._setSimultaneousAccepts()
JakobJingleheimer
left a comment
There was a problem hiding this comment.
This looks good to me! I see there is some uncertainty (#173 (comment)) about the proper handling for this, so let's get someone from @nodejs/net to chime in before landing.
PS Sorry this took me a while to get back to.
| /** | ||
| * Collects all import/require statements for 'node:net' | ||
| */ | ||
| function getAllNetImportStatements(root: SgRoot<Js>): SgNode<Js>[] { | ||
| return [ | ||
| ...getNodeImportStatements(root, 'net'), | ||
| ...getNodeImportCalls(root, 'net'), | ||
| ...getNodeRequireCalls(root, 'net'), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
After this PR was opened, we added this as a built-in utility: @nodejs/codemod-utils:getModuleDependencies
| const netImportStatements = getAllNetImportStatements(root); | ||
|
|
||
| // If no import found we don't process the file | ||
| if (!netImportStatements.length) return null; | ||
|
|
||
| for (const statement of netImportStatements) { | ||
| processNetImportStatement(rootNode, statement, linesToRemove, edits); | ||
| } |
There was a problem hiding this comment.
The length check is unnecessary: getAllNetImportStatements (and @nodejs/codemod-utils:getModuleDependencies) always return an array, so the for…of won't break and will simply do nothing when the array is empty; then the check right after will handle aborting, affectively achieving the same thing with less code.
| const netImportStatements = getAllNetImportStatements(root); | |
| // If no import found we don't process the file | |
| if (!netImportStatements.length) return null; | |
| for (const statement of netImportStatements) { | |
| processNetImportStatement(rootNode, statement, linesToRemove, edits); | |
| } | |
| for (const statement of getAllNetImportStatements(root)) { | |
| processNetImportStatement(rootNode, statement, linesToRemove, edits); | |
| } |
| const netImportStatements = getAllNetImportStatements(root); | |
| // If no import found we don't process the file | |
| if (!netImportStatements.length) return null; | |
| for (const statement of netImportStatements) { | |
| processNetImportStatement(rootNode, statement, linesToRemove, edits); | |
| } | |
| for (const statement of getModuleDependencies(root)) { | |
| processNetImportStatement(rootNode, statement, linesToRemove, edits); | |
| } |
| import { | ||
| getNodeImportStatements, | ||
| getNodeImportCalls, | ||
| } from '@nodejs/codemod-utils/ast-grep/import-statement'; | ||
| import { getNodeRequireCalls } from '@nodejs/codemod-utils/ast-grep/require-call'; |
There was a problem hiding this comment.
nit (see below)
| import { | |
| getNodeImportStatements, | |
| getNodeImportCalls, | |
| } from '@nodejs/codemod-utils/ast-grep/import-statement'; | |
| import { getNodeRequireCalls } from '@nodejs/codemod-utils/ast-grep/require-call'; | |
| import { getModuleDependencies } from '@nodejs/codemod-utils/ast-grep/module-dependencies'; |
| if (argNode) { | ||
| handleCallArgument(rootNode, argNode, linesToRemove); | ||
| } |
There was a problem hiding this comment.
nit:
| if (argNode) { | |
| handleCallArgument(rootNode, argNode, linesToRemove); | |
| } | |
| if (argNode) handleCallArgument(rootNode, argNode, linesToRemove); |
| case 'member_expression': handleMemberExpressionArgument(rootNode, argNode, linesToRemove); break; | ||
| case 'identifier': handleIdentifierArgument(rootNode, argNode, linesToRemove); break; |
There was a problem hiding this comment.
There's some extra indentation on the second case which at first glance makes it look like nested code.
A nit line-break for the break so it doesn't get (visually) lost and look like fall-through.
| case 'member_expression': handleMemberExpressionArgument(rootNode, argNode, linesToRemove); break; | |
| case 'identifier': handleIdentifierArgument(rootNode, argNode, linesToRemove); break; | |
| case 'member_expression': handleMemberExpressionArgument(rootNode, argNode, linesToRemove); | |
| break; | |
| case 'identifier': handleIdentifierArgument(rootNode, argNode, linesToRemove); | |
| break; |
| index: endPos + 1, | ||
| column: range.end.column + 1 | ||
| } |
There was a problem hiding this comment.
nit (formatting/linting)
| index: endPos + 1, | |
| column: range.end.column + 1 | |
| } | |
| column: range.end.column + 1, | |
| index: endPos + 1, | |
| }, |
| if (topLevelStatement) { | ||
| linesToRemove.push(topLevelStatement.range()); | ||
| } |
There was a problem hiding this comment.
nit
| if (topLevelStatement) { | |
| linesToRemove.push(topLevelStatement.range()); | |
| } | |
| if (topLevelStatement) linesToRemove.push(topLevelStatement.range()); |
| function findTopLevelStatement(node: SgNode<Js>): SgNode<Js> | null { | ||
| let current: SgNode<Js> | null = node; | ||
|
|
||
| while (current) { | ||
| const parent = current.parent(); | ||
| if (!parent) break; | ||
|
|
||
| if (parent.kind() === 'program') { | ||
| return current; | ||
| } | ||
|
|
||
| current = parent; | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
I think this can be simplified:
| function findTopLevelStatement(node: SgNode<Js>): SgNode<Js> | null { | |
| let current: SgNode<Js> | null = node; | |
| while (current) { | |
| const parent = current.parent(); | |
| if (!parent) break; | |
| if (parent.kind() === 'program') { | |
| return current; | |
| } | |
| current = parent; | |
| } | |
| return null; | |
| } | |
| function findTopLevelStatement(node: SgNode<Js>): SgNode<Js> | null { | |
| let current: SgNode<Js> | null = node; | |
| while (current = current.parent()) { | |
| if (current?.kind() === 'program') return current; | |
| } | |
| return null; | |
| } |
I checked with a quick test:
function generateNode(kind) {
let i = 1;
return {
parent() {
if (i++ < 4) return {
kind() { return kind },
};
return null;
},
};
}
findTopLevelStatement(generateNode('not-program'); // null
findTopLevelStatement(generateNode('program'); // { kind: 𝑓 }There was a problem hiding this comment.
And alternative that I personally prefer is use a query, that one check if this is inside of a program independent of the level/distance
const program = match.find({
rule: {
inside: {
kind: 'program',
stopBy: 'end',
},
}
})| "codemod", | ||
| "migrations", | ||
| "node.js" | ||
| "node.js" |
There was a problem hiding this comment.
| "node.js" | |
| "node.js" |
|
I asked @jasnell to take a look, and he said
So I think we're g2g here |
|
@vespa7 CI is angry about package-lock. I'm not sure why. |
There was a problem hiding this comment.
Pull request overview
Adds a new codemod recipe to remove deprecated net._setSimultaneousAccepts() calls (DEP0121) from JS/TS codebases, including fixture coverage for CommonJS, ESM, and dynamic imports.
Changes:
- Introduce
net-setSimultaneousAccepts-migrationrecipe (workflow, transform, metadata, README, and fixtures). - Add input/expected fixtures validating call removal and some unused-argument cleanup.
- Register the new recipe in the repo’s lockfile and adjust root
package.jsonformatting.
Reviewed changes
Copilot reviewed 35 out of 37 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| recipes/net-setSimultaneousAccepts-migration/workflow.yaml | Workflow entry to run the new transform via js-ast-grep. |
| recipes/net-setSimultaneousAccepts-migration/src/workflow.ts | Transform implementation removing _setSimultaneousAccepts calls and some related unused declarations/properties. |
| recipes/net-setSimultaneousAccepts-migration/tests/input/* | New fixtures covering various import styles and call locations. |
| recipes/net-setSimultaneousAccepts-migration/tests/expected/* | Expected outputs after the codemod runs. |
| recipes/net-setSimultaneousAccepts-migration/package.json | Recipe package metadata and test script. |
| recipes/net-setSimultaneousAccepts-migration/codemod.yaml | Codemod registry metadata for the recipe. |
| recipes/net-setSimultaneousAccepts-migration/README.md | Usage/examples documentation for the recipe. |
| package.json | Minor formatting change in keywords array. |
| package-lock.json | Adds a linked workspace entry for the new recipe package. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| "author": "Alejandro Espa", | ||
| "license": "MIT", | ||
| "homepage": "https://github.com/nodejs/userland-migrations/blob/main/recipes/net-setsimultaneousaccepts-migration/README.md", |
There was a problem hiding this comment.
homepage URL path has the same directory casing mismatch as repository.directory, so the generated link will 404. Update it to point at recipes/net-setSimultaneousAccepts-migration/README.md.
| "homepage": "https://github.com/nodejs/userland-migrations/blob/main/recipes/net-setsimultaneousaccepts-migration/README.md", | |
| "homepage": "https://github.com/nodejs/userland-migrations/blob/main/recipes/net-setSimultaneousAccepts-migration/README.md", |
| if (expressionStatement) { | ||
| linesToRemove.push(expressionStatement.range()); | ||
| } else { | ||
| edits.push(callNode.replace('')); |
There was a problem hiding this comment.
When _setSimultaneousAccepts() appears in a non-statement context (e.g. initializer/argument), removeCallExpression falls back to callNode.replace(''), which can leave invalid syntax (e.g. const x = ; or fn(, y)). This needs a syntactically-safe replacement (e.g. void 0) or logic to remove/repair the surrounding construct instead of emitting an empty string.
| edits.push(callNode.replace('')); | |
| edits.push(callNode.replace('void 0')); |
There was a problem hiding this comment.
I think this is nonsense…
| kind: 'pair', | ||
| has: { | ||
| field: 'key', | ||
| regex: propertyName, |
There was a problem hiding this comment.
Property removal matches object keys using regex: propertyName, which can accidentally match superset keys (e.g. port matches portNumber) and can misbehave if propertyName ever contains regex metacharacters. Prefer matching the key as a property_identifier with an anchored regex (^...$) or an exact pattern match.
| regex: propertyName, | |
| pattern: propertyName, |
There was a problem hiding this comment.
This sounds like a good suggestion, but I'm not that familiar with JSSG. I see an example in the docs that seems to suggest this would work: https://docs.codemod.com/jssg/reference#basic-example
@brunocroh thoughts?
There was a problem hiding this comment.
Here is the documentation about it: https://ast-grep.github.io/guide/rule-config/atomic-rule.html#regex
Yeah, normally I try to suggest and use pattern instead of regex, but I’m experimenting with this scenario and for some reason, it isn’t working as intended. That’s fine, we can keep it this way for this case.
I’ll try to take a deeper dive into it; maybe it’s an issue on the codemod/ast-grep.
| name: Apply AST Transformations | ||
| type: automatic | ||
| steps: | ||
| - name: Handle DEDEP0121 Remove net._setSimultaneousAccepts(). |
There was a problem hiding this comment.
Step name has a typo in the deprecation identifier (DEDEP0121). This should be DEP0121 to match the Node.js deprecation code and the rest of the recipe metadata/searchability.
| schema_version: "1.0" | ||
| name: "@nodejs/net-setSimultaneousAccepts-deprecation" | ||
| version: 1.0.0 | ||
| description: "Handle DEDEP0121: Remove net._setSimultaneousAccepts()" |
There was a problem hiding this comment.
codemod.yaml metadata has the same typo (DEDEP0121). Please change to DEP0121 so the recipe description is accurate and consistent with the Node.js deprecation identifier.
| description: "Handle DEDEP0121: Remove net._setSimultaneousAccepts()" | |
| description: "Handle DEP0121: Remove net._setSimultaneousAccepts()" |
| "directory": "recipes/net-setsimultaneousaccepts-migration", | ||
| "bugs": "https://github.com/nodejs/userland-migrations/issues" | ||
| }, | ||
| "author": "Alejandro Espa", | ||
| "license": "MIT", | ||
| "homepage": "https://github.com/nodejs/userland-migrations/blob/main/recipes/net-setsimultaneousaccepts-migration/README.md", |
There was a problem hiding this comment.
repository.directory does not match this recipe’s actual folder path (recipes/net-setSimultaneousAccepts-migration). The current value uses a different casing (net-setsimultaneousaccepts-migration), which will produce broken links in npm/GitHub metadata.
| "directory": "recipes/net-setsimultaneousaccepts-migration", | |
| "bugs": "https://github.com/nodejs/userland-migrations/issues" | |
| }, | |
| "author": "Alejandro Espa", | |
| "license": "MIT", | |
| "homepage": "https://github.com/nodejs/userland-migrations/blob/main/recipes/net-setsimultaneousaccepts-migration/README.md", | |
| "directory": "recipes/net-setSimultaneousAccepts-migration", | |
| "bugs": "https://github.com/nodejs/userland-migrations/issues" | |
| }, | |
| "author": "Alejandro Espa", | |
| "license": "MIT", | |
| "homepage": "https://github.com/nodejs/userland-migrations/blob/main/recipes/net-setSimultaneousAccepts-migration/README.md", |
| "codemod", | ||
| "migrations", | ||
| "node.js" | ||
| "node.js" |
There was a problem hiding this comment.
Trailing whitespace after the "node.js" keyword entry will cause noisy diffs / potential lint failures. Please remove the extra space at end of the line.
| "node.js" | |
| "node.js" |
| 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: propertyName, | ||
| }, | ||
| }, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
I think this one can be simplified using relational rules, like inside.
https://ast-grep.github.io/guide/rule-config/relational-rule.html#relational-rules
const pairs = rootNode.findAll({
rule: {
kind: 'pair',
has: {
field: 'key',
regex: propertyName,
},
inside: {
kind: 'variable_declarator',
has: {
kind: 'identifier',
field: 'name',
pattern: objectName,
},
stopBy: 'end',
},
},
});
Closes #173