Skip to content

Conversation

nekojanai
Copy link
Contributor

@nekojanai nekojanai commented Sep 10, 2025

closes #98

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.

Noice! But my advice is to start writing test commit them on local then build the codemod and then push everything

"author": "nekojanai (Jana)",
"license": "MIT",
"homepage": "https://github.com/nodejs/userland-migrations/blob/main/recipes/buffer-atob-btoa/README.md",
"dependencies": {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe need to add jssg types as devdep to have typing on codemod context

Copy link
Member

Choose a reason for hiding this comment

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

@nekojanai you have to add it for strictness

@nekojanai nekojanai force-pushed the feat(`migrate-legacy-buffer-atob-btoa`) branch from 23a2fcf to 2c2fee2 Compare September 17, 2025 16:34
@nekojanai nekojanai force-pushed the feat(`migrate-legacy-buffer-atob-btoa`) branch from 2c2fee2 to f6f823e Compare September 17, 2025 16:37
@nekojanai nekojanai marked this pull request as ready for review September 17, 2025 16:38
@nekojanai nekojanai requested a review from brunocroh September 17, 2025 16:40
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.

Missing test/feature:

  • dynamic import
  • esm import

@nekojanai
Copy link
Contributor Author

@AugustinMauroy Could you provide examples for dynamic and esm imports to test against?

@AugustinMauroy
Copy link
Member

ESM import

Input/before:

import buffer from "node:buffer";

// do stuff

Dynamic import

const buffer = await import("node:buffer");

// do stuff

Don't worry it's shouldn't complicated because we have serval utilities for that

@JakobJingleheimer JakobJingleheimer added the awaiting author Reviewer has requested something from the author label Sep 24, 2025
@nekojanai nekojanai requested a review from brunocroh September 30, 2025 19:37
@nekojanai
Copy link
Contributor Author

Should be gtm

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

@brunocroh brunocroh left a comment

Choose a reason for hiding this comment

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

Looks great now, Thank you!

}
];

const statements = [...getNodeRequireCalls(root, 'buffer'), ...getNodeImportStatements(root, 'buffer')];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const statements = [...getNodeRequireCalls(root, 'buffer'), ...getNodeImportStatements(root, 'buffer')];
const statements = [
...getNodeRequireCalls(root, 'buffer'),
...getNodeImportStatements(root, 'buffer')
];


export default function transform(root: SgRoot<Js>): string | null {
const rootNode = root.root();
const bindingStatementFnTuples: [string, SgNode<Js, Kinds<Js>>, (arg: 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.

Suggested change
const bindingStatementFnTuples: [string, SgNode<Js, Kinds<Js>>, (arg: string) => string][] = [];
const bindingStatementFnTuples: [string, SgNode<Js>, (arg: string) => string][] = [];

this type can be simplified

}
});

const otherCalls = rootNode.findAll({
Copy link
Member

Choose a reason for hiding this comment

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

if you remove that it's should work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But do I not need to check for any other calls to an import before deleting it?

Copy link
Member

Choose a reason for hiding this comment

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

Oh make sense maybe add comment on top of this query

linesToRemove.push(statement.range());
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!edits.lenght) return null

@AugustinMauroy AugustinMauroy added the awaiting author Reviewer has requested something from the author label Oct 1, 2025
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: Migrate legacy buffer.atob(data) and buffer.btoa(data) APIs
4 participants