Skip to content

Conversation

lluisemper
Copy link

Add codemod to handle deprecated SlowBuffer API migration covering CommonJS, ES6 imports, and usage patterns

Closes: #125 (comment)

@lluisemper lluisemper force-pushed the main branch 4 times, most recently from b04a60b to d2d54d6 Compare August 28, 2025 20:56
Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

WOW, thank you and congratulations on your first contribution.
Here is the first wave of reviews. Overall, it's good. You've covered a lot of cases except for dynamic imports. For that, you have the getNodeImportCalls function, which allows you to query all nodes that are dynamic imports.
As mentioned in one of the comments, you're using a lot of regex. It's better to base it on the AST for greater stability. Also, as mentioned in a message, you have tools to visualise things, and for query examples, you have utility functions that are quite complex.

2. Direct `SlowBuffer` calls to `Buffer.allocUnsafeSlow()`
3. Import/require statements to use `Buffer` instead of `SlowBuffer`

## Examples
Copy link
Member

Choose a reason for hiding this comment

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

example on rest of codemods use this structure :

## Examples

**Before:**
<!-- code snippet  -->

**After:**

<!-- code snippet  -->

I think your aproach is better in terms of semantics.

@nodejs/userland-migrations what should we do modify this pr or update other codemod ?

Copy link
Author

Choose a reason for hiding this comment

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

I think it’s fine to just update this PR for now. In the future, it might be a good idea to add a script or system at the project root that, when run with
npm run new:codemon --name='slow-buffer-to-buffer-alloc-unsafe-slow',
generates a template project for implementing a new codemon. New contributors and existing ones would benefit from it to have all the basic down.

If this template project would have a different structure, it would be then a good opportunity to correct existing codemons.

Let me know if you find this idea interesting and I can expand on it, or we can discuss how it could be implemented.

Copy link
Member

Choose a reason for hiding this comment

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

oh interesting point.

  • Codemod CLI ofter a codemod init but it's don't really cover our code style/architecture.
  • We can have a script that generate the basic of a codemod.
  • Simple way copy paste a codemod (what I do now)

* For ESM imports: { SlowBuffer as SB } -> { Buffer as SB }
* Preserves original formatting as much as possible
*/
function transformDestructuringPattern(originalText: string): string {
Copy link
Member

Choose a reason for hiding this comment

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

Here it's use too much regex, we need to relie on AST for reliability. If you have difficult to visualize the ast you can use Codemod Studio, ast-grep playground or any Tree-sitter sitter visualization tools.

/**
* Process comments mentioning SlowBuffer and update them
*/
function processSlowBufferComments(rootNode: SgNode, edits: Edit[]): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Good idea but in practice we doesn't want to update the user comment. But you can console.warn + file://<path>:<line> where a comment mention this api.

@AugustinMauroy
Copy link
Member

for the red ci just run npm I to add your new recipes to the package-lock.json

@lluisemper lluisemper force-pushed the main branch 3 times, most recently from a68a702 to 48798a1 Compare August 29, 2025 13:51
@lluisemper
Copy link
Author

lluisemper commented Aug 29, 2025

@AugustinMauroy thank you very much for the review 💪. I have commited the suggestions and marked the addressed comments with a thums up emoji.

The comments that have a requested change, do not have any emoji. I will aim to commit this over the weekend.

@AugustinMauroy
Copy link
Member

@AugustinMauroy thank you very much for the review 💪. I have commited the suggestions and marked the addressed comments with a thums up emoji.

The comments that have a requested change, do not have any emoji. I will aim to commit this over the weekend.

Wow super! also any pressure we are open source.

Also for your information you have a button with "resolve conversation" which is collapsing the review 😁

Comment on lines 103 to 92
if (
originalText.includes("SlowBuffer") &&
!originalText.includes(",") &&
!originalText.includes(":") &&
!originalText.includes(" as ")
) {
return originalText.replace(/SlowBuffer/g, "Buffer");
}

let newText = originalText;

// Handle aliased patterns (both CommonJS : and ESM as syntax)
// { SlowBuffer: SB } -> { Buffer: SB }
newText = newText.replace(/SlowBuffer(\s*:\s*\w+)/g, "Buffer$1");
// { SlowBuffer as SB } -> { Buffer as SB }
newText = newText.replace(/SlowBuffer(\s+as\s+\w+)/g, "Buffer$1");

// If Buffer is already present in this specific pattern, just remove SlowBuffer
if (originalText.includes("Buffer") && originalText.includes("SlowBuffer")) {
// Remove non-aliased SlowBuffer references very carefully to preserve spacing
newText = newText
.replace(/,\s*SlowBuffer(?!\s*[:as])/g, "") // Remove SlowBuffer with leading comma
.replace(/SlowBuffer\s*,\s*/g, "") // Remove SlowBuffer with trailing comma and space
.replace(/SlowBuffer(?!\s*[:as])/g, ""); // Remove standalone SlowBuffer (not followed by : or as)

// Clean up any double spaces after opening brace
newText = newText.replace(/{\s{2,}/g, "{ ");
// Clean up any double commas but preserve spacing
newText = newText.replace(/,\s*,/g, ",");
}
// If Buffer is not present, replace first SlowBuffer with Buffer, remove others
else if (originalText.includes("SlowBuffer")) {
// Replace the first non-aliased SlowBuffer with Buffer
newText = newText.replace(/SlowBuffer(?!\s*[:as])/, "Buffer");
// Remove any remaining non-aliased SlowBuffer references
newText = newText
.replace(/,\s*SlowBuffer\s*(?![\s:as])/g, "")
.replace(/SlowBuffer\s*,\s*/g, "")
.replace(/SlowBuffer\s*(?![\s:as])/g, "");
// Clean up commas
newText = newText.replace(/,\s*,/g, ",");
}

return newText;
}
Copy link
Member

Choose a reason for hiding this comment

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

We have a utility function that resolves these scenarios

please check utils/ast-grep/resolve-binding-path.ts

Comment on lines 159 to 253
const objectPatterns = rootNode.findAll({ rule: { kind: "object_pattern" } });
for (const pattern of objectPatterns) {
const text = pattern.text();
// Match patterns like { SlowBuffer: SB } to extract alias "SB"
const aliasMatches = text.matchAll(/SlowBuffer\s*:\s*(\w+)/g);
for (const match of aliasMatches) {
slowBufferAliases.add(match[1]);
}
}

// Find all named_imports that include SlowBuffer aliases (ESM imports)
const namedImports = rootNode.findAll({ rule: { kind: "named_imports" } });
for (const importNode of namedImports) {
const text = importNode.text();
// Match patterns like { SlowBuffer as SB } to extract alias "SB"
const aliasMatches = text.matchAll(/SlowBuffer\s+as\s+(\w+)/g);
for (const match of aliasMatches) {
slowBufferAliases.add(match[1]);
}
}

// Handle constructor patterns: new SlowBuffer(size) and new SB(size) for aliases
const constructorCalls = rootNode.findAll({
rule: {
kind: "new_expression",
has: {
field: "constructor",
kind: "identifier",
regex: "^SlowBuffer$",
},
},
});

for (const constructorCall of constructorCalls) {
const args = constructorCall.field("arguments");
if (args) {
// Extract the arguments text (without parentheses)
const argsText = args.text().slice(1, -1); // Remove ( and )
edits.push(constructorCall.replace(`Buffer.allocUnsafeSlow(${argsText})`));
hasChanges = true;
}
}

// Handle constructor calls with aliases
for (const alias of slowBufferAliases) {
const aliasConstructorCalls = rootNode.findAll({
rule: {
kind: "new_expression",
has: {
field: "constructor",
kind: "identifier",
regex: `^${alias}$`,
},
},
});

for (const constructorCall of aliasConstructorCalls) {
const args = constructorCall.field("arguments");
if (args) {
const argsText = args.text().slice(1, -1);
edits.push(constructorCall.replace(`${alias}.allocUnsafeSlow(${argsText})`));
hasChanges = true;
}
}
}

// Handle direct function calls: SlowBuffer(size)
const directCalls = rootNode.findAll({
rule: {
kind: "call_expression",
has: {
field: "function",
kind: "identifier",
regex: "^SlowBuffer$",
},
},
});

for (const directCall of directCalls) {
const args = directCall.field("arguments");
if (args) {
// Extract the arguments text (without parentheses)
const argsText = args.text().slice(1, -1); // Remove ( and )
edits.push(directCall.replace(`Buffer.allocUnsafeSlow(${argsText})`));
hasChanges = true;
}
}

// Handle direct function calls with aliases
for (const alias of slowBufferAliases) {
const aliasDirectCalls = rootNode.findAll({
rule: {
kind: "call_expression",
has: {
field: "function",
kind: "identifier",
regex: `^${alias}$`,
},
},
});

for (const directCall of aliasDirectCalls) {
const args = directCall.field("arguments");
if (args) {
const argsText = args.text().slice(1, -1);
edits.push(directCall.replace(`${alias}.allocUnsafeSlow(${argsText})`));
hasChanges = true;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This scenario can be simplified using utils/resolve-binding-path, and improving findAll from ast-grep with any and multiple matches.

Suggested change
const objectPatterns = rootNode.findAll({ rule: { kind: "object_pattern" } });
for (const pattern of objectPatterns) {
const text = pattern.text();
// Match patterns like { SlowBuffer: SB } to extract alias "SB"
const aliasMatches = text.matchAll(/SlowBuffer\s*:\s*(\w+)/g);
for (const match of aliasMatches) {
slowBufferAliases.add(match[1]);
}
}
// Find all named_imports that include SlowBuffer aliases (ESM imports)
const namedImports = rootNode.findAll({ rule: { kind: "named_imports" } });
for (const importNode of namedImports) {
const text = importNode.text();
// Match patterns like { SlowBuffer as SB } to extract alias "SB"
const aliasMatches = text.matchAll(/SlowBuffer\s+as\s+(\w+)/g);
for (const match of aliasMatches) {
slowBufferAliases.add(match[1]);
}
}
// Handle constructor patterns: new SlowBuffer(size) and new SB(size) for aliases
const constructorCalls = rootNode.findAll({
rule: {
kind: "new_expression",
has: {
field: "constructor",
kind: "identifier",
regex: "^SlowBuffer$",
},
},
});
for (const constructorCall of constructorCalls) {
const args = constructorCall.field("arguments");
if (args) {
// Extract the arguments text (without parentheses)
const argsText = args.text().slice(1, -1); // Remove ( and )
edits.push(constructorCall.replace(`Buffer.allocUnsafeSlow(${argsText})`));
hasChanges = true;
}
}
// Handle constructor calls with aliases
for (const alias of slowBufferAliases) {
const aliasConstructorCalls = rootNode.findAll({
rule: {
kind: "new_expression",
has: {
field: "constructor",
kind: "identifier",
regex: `^${alias}$`,
},
},
});
for (const constructorCall of aliasConstructorCalls) {
const args = constructorCall.field("arguments");
if (args) {
const argsText = args.text().slice(1, -1);
edits.push(constructorCall.replace(`${alias}.allocUnsafeSlow(${argsText})`));
hasChanges = true;
}
}
}
// Handle direct function calls: SlowBuffer(size)
const directCalls = rootNode.findAll({
rule: {
kind: "call_expression",
has: {
field: "function",
kind: "identifier",
regex: "^SlowBuffer$",
},
},
});
for (const directCall of directCalls) {
const args = directCall.field("arguments");
if (args) {
// Extract the arguments text (without parentheses)
const argsText = args.text().slice(1, -1); // Remove ( and )
edits.push(directCall.replace(`Buffer.allocUnsafeSlow(${argsText})`));
hasChanges = true;
}
}
// Handle direct function calls with aliases
for (const alias of slowBufferAliases) {
const aliasDirectCalls = rootNode.findAll({
rule: {
kind: "call_expression",
has: {
field: "function",
kind: "identifier",
regex: `^${alias}$`,
},
},
});
for (const directCall of aliasDirectCalls) {
const args = directCall.field("arguments");
if (args) {
const argsText = args.text().slice(1, -1);
edits.push(directCall.replace(`${alias}.allocUnsafeSlow(${argsText})`));
hasChanges = true;
}
}
}
var ImportNode // importNode can be fetch with getNodeImportStatement or getNodeRequireCalls
const binding = resolveBindPath(importN, "$.SlowBuffer") //it will resolve the name of the SlowBuffer in this file, independent of the way it was imported.
const aliasConstructorCalls = rootNode.findAll({
rule: {
any: [
{
kind: "new_expression",
pattern: `new ${binding}($$$ARGS)`,
},
{
kind: "call_expression",
pattern: `${binding}($$$ARGS)`,
},
],
},
});
for(const match of matches) {
if(match.kind() === 'new_expression') { // each scenario can be handled by kind here if needed
const args = match.getMultipleMatches() //get args
edits.push(match.replace(/* handle replace */)
}
}

Comment on lines 103 to 146
if (
originalText.includes("SlowBuffer") &&
!originalText.includes(",") &&
!originalText.includes(":") &&
!originalText.includes(" as ")
) {
return originalText.replace(/SlowBuffer/g, "Buffer");
}

let newText = originalText;

// Handle aliased patterns (both CommonJS : and ESM as syntax)
// { SlowBuffer: SB } -> { Buffer: SB }
newText = newText.replace(/SlowBuffer(\s*:\s*\w+)/g, "Buffer$1");
// { SlowBuffer as SB } -> { Buffer as SB }
newText = newText.replace(/SlowBuffer(\s+as\s+\w+)/g, "Buffer$1");

// If Buffer is already present in this specific pattern, just remove SlowBuffer
if (originalText.includes("Buffer") && originalText.includes("SlowBuffer")) {
// Remove non-aliased SlowBuffer references very carefully to preserve spacing
newText = newText
.replace(/,\s*SlowBuffer(?!\s*[:as])/g, "") // Remove SlowBuffer with leading comma
.replace(/SlowBuffer\s*,\s*/g, "") // Remove SlowBuffer with trailing comma and space
.replace(/SlowBuffer(?!\s*[:as])/g, ""); // Remove standalone SlowBuffer (not followed by : or as)

// Clean up any double spaces after opening brace
newText = newText.replace(/{\s{2,}/g, "{ ");
// Clean up any double commas but preserve spacing
newText = newText.replace(/,\s*,/g, ",");
}
// If Buffer is not present, replace first SlowBuffer with Buffer, remove others
else if (originalText.includes("SlowBuffer")) {
// Replace the first non-aliased SlowBuffer with Buffer
newText = newText.replace(/SlowBuffer(?!\s*[:as])/, "Buffer");
// Remove any remaining non-aliased SlowBuffer references
newText = newText
.replace(/,\s*SlowBuffer\s*(?![\s:as])/g, "")
.replace(/SlowBuffer\s*,\s*/g, "")
.replace(/SlowBuffer\s*(?![\s:as])/g, "");
// Clean up commas
newText = newText.replace(/,\s*,/g, ",");
}

return newText;
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 it can be handled with updateBinding utility function #182 , but it hasn’t been merged yet.

@AugustinMauroy Could we check with someone to follow up on the #182 merge?

Copy link
Member

Choose a reason for hiding this comment

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

@AugustinMauroy Could we check with someone to follow up on the #182 merge? yess

@lluisemper
Copy link
Author

Hey guys! Based on your and suggestiongs I have pushed some changes. I have included some test for dynamic imports and I decided to remove the warning for the comments, with all the suggestions it was increasing the complexity and I was loosing track of what was what.

In addition to this I am not sure if updating this branch with a commit merge from main is fine or if you would prefer to rebase it. I will leave it down to you

@AugustinMauroy
Copy link
Member

Hey guys! Based on your and suggestiongs I have pushed some changes. I have included some test for dynamic imports and I decided to remove the warning for the comments, with all the suggestions it was increasing the complexity and I was loosing track of what was what.

In addition to this I am not sure if updating this branch with a commit merge from main is fine or if you would prefer to rebase it. I will leave it down to you

It's look good I am still on holiday I am going to review it when I'll be back !

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

LGMT !

@lluisemper
Copy link
Author

@AugustinMauroy @brunocroh Thank you very much for the reviews! I managed to learn a lot.
By the way, I have commited the change in which I was removing the hasChanged variables which were usseles. This has caused that approval is needed once again. However I will leave this in your hands.

@AugustinMauroy
Copy link
Member

@AugustinMauroy @brunocroh Thank you very much for the reviews! I managed to learn a lot. By the way, I have commited the change in which I was removing the hasChanged variables which were usseles. This has caused that approval is needed once again. However I will leave this in your hands.

It's good now ! I'm just waiting the @JakobJingleheimer (actual formal maintainer) approval to merge and publish

Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

This looks pretty good! A couple small things, and then I think this is good to go.

PS Sorry for the delayed review.

if (!nameField) continue;

// Handle direct assignment: const SlowBuffer = require('buffer').SlowBuffer
if (nameField.kind() === "identifier" && nameField.text() === "SlowBuffer") {
Copy link
Member

@JakobJingleheimer JakobJingleheimer Sep 22, 2025

Choose a reason for hiding this comment

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

I would use a switch statement:

Suggested change
if (nameField.kind() === "identifier" && nameField.text() === "SlowBuffer") {
switch (nameField.kind()) {
case "identifier":
if (nameField.text() === "SlowBuffer") {
// …
case "object_pattern":

* @param root - The AST root to search for require statements
* @param edits - Array to collect edit operations
*/
function processRequireStatements(root: SgRoot, edits: Edit[]): void {
Copy link
Member

Choose a reason for hiding this comment

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

This function and the other 2 are almost exactly the same. Could they be DRY'd up?

type StatementType = 'import-dynamic' | 'import-static' | 'require';

const nodeGetterMap = {
  'import-dynamic': ,
  'import-static': 
  'require': getNodeRequireCalls,
} as const satisfies Record<StatementType, Function>;

function processStatements(
  root: ,
  edits: ,
  type: StatementType,
) {
  const statements = nodeGetterMap[type](root, 'buffer');
  // I think you can keep the rest the same
}

Copy link
Author

Choose a reason for hiding this comment

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

I think it makes the code less readable by adding this abstraction and incresses the complexity of the code base.
It is not like this is a system that will need to be scalable, it is just like a script that will run once.

I think that this change is not needed. Not sure why would you want to do this when it is as simple as it can get.

Copy link
Member

Choose a reason for hiding this comment

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

I can see arguments for both ways. My rationale is:

When consolidated, you're certain it's doing the same thing and there are no subtle differences (which a human may not even notice).

When duplicated, they can drift over time, very often in unimportant ways, like optional chaining gets added to one but not the other and then you're not sure which is correct or whether they're truly supposed to be different. That "not-sure-if" uncertainty makes the code untouchable.

Consolidated, there's no opportunity for that. So I tend to lean towards a slight increase in complexity to preclude code becoming untouchable.

Copy link
Member

Choose a reason for hiding this comment

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

both is right but I tend to follow Jacob POV

Copy link
Author

Choose a reason for hiding this comment

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

I have implement this, there were 2 functions that were nearly the same. Third one being 'import-static' was quite different.

Copy link
Member

Choose a reason for hiding this comment

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

Could we get a case for aliasing the destructured require too? I believe under the hood it uses the same utility as dynamic import, which has a case, but good to have one for require too to ensure it doesn't get broken.

const { SlowBuffer: SB } = require('buffer');
const buf3 = new SB(256);

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for spotting this. This must be covered. I will add this when I get a chance

Copy link
Author

Choose a reason for hiding this comment

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

While working on it I realized that this test was already covered here: recipes/slow-buffer-to-buffer-alloc-unsafe-slow/tests/expected/file-06.js

@JakobJingleheimer JakobJingleheimer added the awaiting author Reviewer has requested something from the author label Sep 23, 2025
Add codemod to handle deprecated SlowBuffer API migration
covering CommonJS, ES6 imports, and usage patterns

Closes: nodejs#125 (comment)
@lluisemper
Copy link
Author

@JakobJingleheimer I have implemented your small requested changes, let me know what do you think of them

@JakobJingleheimer JakobJingleheimer removed the awaiting author Reviewer has requested something from the author label Sep 25, 2025
@JakobJingleheimer
Copy link
Member

I'm not sure how you were able to bypass the repo's protections and force-push, but that has broken GitHub's review system so I can't review your recent change in isolation—I'll have to do the whole thing over again, which will take a huge amount of time. I probably won't get to it before I'm away next week.

@JakobJingleheimer JakobJingleheimer added the awaiting reviewer Author has responded and needs action from the reviewer label Sep 25, 2025
Comment on lines +46 to +47
if (type === "import-static") {
// ESM imports have a different structure - handle them separately
Copy link
Member

Choose a reason for hiding this comment

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

@brunocroh don't we have a utility for this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we have, but I can see one case here that is not covered by our utility. The new one added in #182 will probably simplify this code a lot. I’ll work on that now so we can use it here.

@lluisemper
Copy link
Author

I'm not sure how you were able to bypass the repo's protections and force-push, but that has broken GitHub's review system so I can't review your recent change in isolation—I'll have to do the whole thing over again, which will take a huge amount of time. I probably won't get to it before I'm away next week.

Oh no, I am so sorry. The bypass of the repo's protection will be because I am in a fork, therefore the original repo setting do not affect it. Could it be a good idea to document this?

I was just trying to avoid a bloated history with many commits.

@JakobJingleheimer
Copy link
Member

JakobJingleheimer commented Sep 25, 2025

Hmm. The docs had previously said it would apply to fork branches (and we had previously tested it). I didn't document it because the error GitHub threw made it fairly obvious. But if the protection is now broken by design, I guess yeah good idea to document it 🫠

I was just trying to avoid a bloated history with many commits.

PRs are squash-merged, so only 1 commit per PR is added to the history.

@JakobJingleheimer JakobJingleheimer added blocked:upstream Depends on another PR or external change/fix and removed awaiting reviewer Author has responded and needs action from the reviewer labels Sep 26, 2025
@AugustinMauroy
Copy link
Member

@JakobJingleheimer @brunocroh still blocked ?

@JakobJingleheimer JakobJingleheimer added awaiting author Reviewer has requested something from the author and removed blocked:upstream Depends on another PR or external change/fix labels Oct 9, 2025
@JakobJingleheimer
Copy link
Member

still blocked ?

No, the upstream blocker is resolved, so this PR can now consume it per Bruno's #193 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting author Reviewer has requested something from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: handle SlowBuffer depreciation
6 participants