-
-
Notifications
You must be signed in to change notification settings - Fork 29
feat(process-assert-to-node-assert): introduce
#200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 15 commits
a96083c
48c4a1d
91b0098
ccec01a
817993b
a9fb7f1
679f077
85589a2
6b636de
f1527c2
c6996a1
fa09574
7b347db
19c7e11
da74a79
d48be7b
aa1c3b6
b37052a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| # `process.assert` to `node:assert` DEP0100 | ||
|
|
||
| This recipe transforms the usage of `process.assert` to use `node:assert` module. | ||
|
|
||
| See [DEP0100](https://nodejs.org/api/deprecations.html#DEP0100). | ||
|
|
||
| ## Example | ||
|
|
||
| **Before:** | ||
|
|
||
| ```js | ||
| process.assert(condition, "Assertion failed"); | ||
| ``` | ||
|
|
||
| **After:** | ||
|
|
||
| ```js | ||
| import assert from "node:assert"; | ||
| assert(condition, "Assertion failed"); | ||
| ``` | ||
|
|
||
| ## Additional Notes | ||
|
|
||
| This codemod use [`fs` capability](https://docs.codemod.com/jssg/security) to read the `package.json` file and determine if the project is using ES modules or CommonJS. Based on this information, it adds the appropriate import statement for the `assert` module. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may not need to do that: |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| schema_version: "1.0" | ||
| name: "@nodejs/process-assert-to-node-assert" | ||
| version: 1.0.0 | ||
| description: Handle DEP0100 via transforming `process.assert` to `node:assert`. | ||
| author: matheusmorett2 | ||
| license: MIT | ||
| workflow: workflow.yaml | ||
| category: migration | ||
|
|
||
| targets: | ||
| languages: | ||
| - javascript | ||
| - typescript | ||
|
|
||
| keywords: | ||
| - transformation | ||
| - migration | ||
|
|
||
| registry: | ||
| access: public | ||
| visibility: public | ||
|
|
||
| # https://docs.codemod.com/jssg/security#enabling-capabilities | ||
| capabilities: | ||
| - fs |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| { | ||
| "name": "@nodejs/process-assert-to-node-assert", | ||
| "version": "1.0.0", | ||
| "description": "Handle DEP0100 via transforming `process.assert` to `node:assert`.", | ||
| "type": "module", | ||
| "scripts": { | ||
| "test": "npx codemod jssg test -l typescript --ignore-whitespace ./src/workflow.ts ./" | ||
| }, | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "git+https://github.com/nodejs/userland-migrations.git", | ||
| "directory": "recipes/process-assert-to-node-assert", | ||
| "bugs": "https://github.com/nodejs/userland-migrations/issues" | ||
| }, | ||
| "author": "matheusmorett2", | ||
| "license": "MIT", | ||
| "homepage": "https://github.com/nodejs/userland-migrations/blob/main/recipes/process-assert-to-node-assert/README.md", | ||
| "devDependencies": { | ||
| "@codemod.com/jssg-types": "^1.0.3" | ||
| }, | ||
| "dependencies": { | ||
| "@nodejs/codemod-utils": "*" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,230 @@ | ||||||||||||||||
| import { EOL } from 'node:os'; | ||||||||||||||||
| import { readFileSync } from 'node:fs'; | ||||||||||||||||
| import { join } from 'node:path'; | ||||||||||||||||
| import { getNodeRequireCalls } from '@nodejs/codemod-utils/ast-grep/require-call'; | ||||||||||||||||
| import { getNodeImportStatements } from '@nodejs/codemod-utils/ast-grep/import-statement'; | ||||||||||||||||
| import { resolveBindingPath } from '@nodejs/codemod-utils/ast-grep/resolve-binding-path'; | ||||||||||||||||
| import { removeBinding } from '@nodejs/codemod-utils/ast-grep/remove-binding'; | ||||||||||||||||
| import { removeLines } from '@nodejs/codemod-utils/ast-grep/remove-lines'; | ||||||||||||||||
| import type { | ||||||||||||||||
| Edit, | ||||||||||||||||
| Range, | ||||||||||||||||
| Rule, | ||||||||||||||||
| SgNode, | ||||||||||||||||
| SgRoot, | ||||||||||||||||
| } from '@codemod.com/jssg-types/main'; | ||||||||||||||||
| import type JS from '@codemod.com/jssg-types/langs/javascript'; | ||||||||||||||||
|
|
||||||||||||||||
| type ReplaceRule = { | ||||||||||||||||
| importNode?: SgNode<JS>; | ||||||||||||||||
| binding?: string; | ||||||||||||||||
| rule: Rule<JS>; | ||||||||||||||||
| replaceWith?: string; | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| /** | ||||||||||||||||
| * Transform function that converts deprecated `process.assert` usage to the `node:assert` module. | ||||||||||||||||
| * | ||||||||||||||||
| * Handles: | ||||||||||||||||
| * 1. Replaces `process.assert(...)` member expressions/calls with `assert(...)` or `assert.xxx` as appropriate. | ||||||||||||||||
| * 2. Handles cases where `process` is imported/required under a different binding (resolves binding paths). | ||||||||||||||||
| * 3. Removes the original `process` import/require when it's only used for `assert` and removes the import line when empty. | ||||||||||||||||
| * 4. Adds `import assert from "node:assert";` or `const assert = require("node:assert");` at the top | ||||||||||||||||
| * when the file does not already import/require `assert`. | ||||||||||||||||
| * | ||||||||||||||||
| * Steps: | ||||||||||||||||
| * - Find all `process` import/require statements and resolve any binding for `assert`. | ||||||||||||||||
| * - Replace call and member-expression usages that reference `process.assert` (or the resolved binding) with `assert`. | ||||||||||||||||
| * - Remove or update the original import/require for `process` when it's no longer needed. | ||||||||||||||||
| * - If `assert` is not already present, insert the appropriate `import` or `require` line depending on the module style. | ||||||||||||||||
| * | ||||||||||||||||
| * @param root - The AST root node provided by jssg for the file being transformed. | ||||||||||||||||
| * @returns The transformed source code as a string, or `null` when no edits are required. | ||||||||||||||||
| */ | ||||||||||||||||
| export default function transform(root: SgRoot<JS>): string | null { | ||||||||||||||||
| const rootNode = root.root(); | ||||||||||||||||
| const edits: Edit[] = []; | ||||||||||||||||
| const linesToRemove: Range[] = []; | ||||||||||||||||
| const replaceRules: ReplaceRule[] = [ | ||||||||||||||||
| { | ||||||||||||||||
| rule: { | ||||||||||||||||
| kind: 'member_expression', | ||||||||||||||||
| pattern: 'process.assert', | ||||||||||||||||
| }, | ||||||||||||||||
| replaceWith: 'assert', | ||||||||||||||||
| }, | ||||||||||||||||
| ]; | ||||||||||||||||
|
|
||||||||||||||||
| const processImportsToRemove = new Set<SgNode<JS>>(); | ||||||||||||||||
|
|
||||||||||||||||
| const requireCalls = getNodeRequireCalls(root, 'process'); | ||||||||||||||||
| const importStatements = getNodeImportStatements(root, 'process'); | ||||||||||||||||
| const allImports = [...requireCalls, ...importStatements]; | ||||||||||||||||
|
Comment on lines
+60
to
+62
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you also need |
||||||||||||||||
| const processUsages = rootNode.findAll({ | ||||||||||||||||
| rule: { | ||||||||||||||||
| kind: 'member_expression', | ||||||||||||||||
| has: { | ||||||||||||||||
| kind: 'identifier', | ||||||||||||||||
| pattern: 'process', | ||||||||||||||||
| }, | ||||||||||||||||
| }, | ||||||||||||||||
| }); | ||||||||||||||||
|
|
||||||||||||||||
| for (const processImport of allImports) { | ||||||||||||||||
| const binding = resolveBindingPath(processImport, '$.assert'); | ||||||||||||||||
|
|
||||||||||||||||
| if (binding) { | ||||||||||||||||
| // Handle member expressions like nodeAssert.strictEqual | ||||||||||||||||
| replaceRules.push({ | ||||||||||||||||
AugustinMauroy marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
| importNode: processImport, | ||||||||||||||||
| binding, | ||||||||||||||||
| rule: { | ||||||||||||||||
| kind: 'member_expression', | ||||||||||||||||
| has: { | ||||||||||||||||
| kind: binding.includes('.') ? 'member_expression' : 'identifier', | ||||||||||||||||
| pattern: binding, | ||||||||||||||||
| }, | ||||||||||||||||
| }, | ||||||||||||||||
| replaceWith: 'assert', | ||||||||||||||||
| }); | ||||||||||||||||
|
|
||||||||||||||||
| // Handle standalone calls like nodeAssert(...) | ||||||||||||||||
| replaceRules.push({ | ||||||||||||||||
| importNode: processImport, | ||||||||||||||||
| binding, | ||||||||||||||||
| rule: { | ||||||||||||||||
| kind: 'call_expression', | ||||||||||||||||
| has: { | ||||||||||||||||
| kind: 'identifier', | ||||||||||||||||
| field: 'function', | ||||||||||||||||
| pattern: binding, | ||||||||||||||||
| }, | ||||||||||||||||
| }, | ||||||||||||||||
| replaceWith: 'assert', | ||||||||||||||||
| }); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| let hasNonAssertUsage = false; | ||||||||||||||||
| for (const usage of processUsages) { | ||||||||||||||||
| const propertyNode = usage.field('property'); | ||||||||||||||||
| if (propertyNode && propertyNode.text() !== 'assert') { | ||||||||||||||||
| hasNonAssertUsage = true; | ||||||||||||||||
| break; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if (!hasNonAssertUsage && processUsages.length > 0) { | ||||||||||||||||
| processImportsToRemove.add(processImport); | ||||||||||||||||
| linesToRemove.push(processImport.range()); | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| const processedImports = new Set<SgNode<JS>>(); | ||||||||||||||||
|
|
||||||||||||||||
| for (const replaceRule of replaceRules) { | ||||||||||||||||
| const nodes = rootNode.findAll({ | ||||||||||||||||
| rule: replaceRule.rule, | ||||||||||||||||
| }); | ||||||||||||||||
|
|
||||||||||||||||
| for (const node of nodes) { | ||||||||||||||||
| if ( | ||||||||||||||||
| replaceRule.importNode && | ||||||||||||||||
| !processedImports.has(replaceRule.importNode) | ||||||||||||||||
| ) { | ||||||||||||||||
| if (!processImportsToRemove.has(replaceRule.importNode)) { | ||||||||||||||||
| const removeBind = removeBinding( | ||||||||||||||||
| replaceRule.importNode, | ||||||||||||||||
| replaceRule.binding, | ||||||||||||||||
| ); | ||||||||||||||||
|
|
||||||||||||||||
| if (removeBind.edit) { | ||||||||||||||||
| edits.push(removeBind.edit); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if (removeBind.lineToRemove) { | ||||||||||||||||
| linesToRemove.push(removeBind.lineToRemove); | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| processedImports.add(replaceRule.importNode); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if ( | ||||||||||||||||
| replaceRule.rule.kind === 'member_expression' && | ||||||||||||||||
| replaceRule.binding | ||||||||||||||||
| ) { | ||||||||||||||||
| // Replace the object part of member expressions (e.g., nodeAssert.strictEqual -> assert.strictEqual) | ||||||||||||||||
| const objectNode = node.field('object'); | ||||||||||||||||
|
|
||||||||||||||||
| if (objectNode) { | ||||||||||||||||
| edits.push(objectNode.replace('assert')); | ||||||||||||||||
| } | ||||||||||||||||
| } else if ( | ||||||||||||||||
| replaceRule.rule.kind === 'call_expression' && | ||||||||||||||||
| replaceRule.binding | ||||||||||||||||
| ) { | ||||||||||||||||
| // Replace the function identifier in call expressions (e.g., nodeAssert(...) -> assert(...)) | ||||||||||||||||
| const functionNode = node.field('function'); | ||||||||||||||||
|
|
||||||||||||||||
| if (functionNode) { | ||||||||||||||||
| edits.push(functionNode.replace('assert')); | ||||||||||||||||
| } | ||||||||||||||||
| } else { | ||||||||||||||||
| const replaceText = replaceRule.replaceWith || 'assert'; | ||||||||||||||||
| edits.push(node.replace(replaceText)); | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| let sourceCode = rootNode.commitEdits(edits); | ||||||||||||||||
|
|
||||||||||||||||
| sourceCode = removeLines(sourceCode, linesToRemove); | ||||||||||||||||
|
Comment on lines
+178
to
+180
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's kind of weird to set and then immediately overwrite the variable.
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| if (edits.length === 0 && linesToRemove) return sourceCode; | ||||||||||||||||
|
|
||||||||||||||||
| const alreadyRequiringAssert = getNodeRequireCalls(root, 'assert'); | ||||||||||||||||
| const alreadyImportingAssert = getNodeImportStatements(root, 'assert'); | ||||||||||||||||
|
|
||||||||||||||||
| if (alreadyRequiringAssert.length || alreadyImportingAssert.length) | ||||||||||||||||
| return sourceCode; | ||||||||||||||||
|
Comment on lines
+187
to
+188
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| const usingRequire = rootNode.find({ | ||||||||||||||||
| rule: { | ||||||||||||||||
| kind: 'call_expression', | ||||||||||||||||
| has: { | ||||||||||||||||
| kind: 'identifier', | ||||||||||||||||
| field: 'function', | ||||||||||||||||
| regex: 'require', | ||||||||||||||||
| }, | ||||||||||||||||
| }, | ||||||||||||||||
| }); | ||||||||||||||||
| const usingImport = rootNode.find({ | ||||||||||||||||
| rule: { | ||||||||||||||||
| kind: 'import_statement', | ||||||||||||||||
| }, | ||||||||||||||||
| }); | ||||||||||||||||
| const filename = root.filename(); | ||||||||||||||||
| const isCjsFile = filename.endsWith('.cjs'); | ||||||||||||||||
| const isMjsFile = filename.endsWith('.mjs'); | ||||||||||||||||
|
Comment on lines
+206
to
+207
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since node supports typescript by default now, I think this should handle that too:
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| // Prefer adding an ES module import when the file already uses ESM syntax | ||||||||||||||||
| // (contains `import` statements) or is an `.mjs` file. This avoids injecting a | ||||||||||||||||
| // CommonJS `require` into an ES module source (even if the file references | ||||||||||||||||
| // `createRequire`). | ||||||||||||||||
| if (usingImport || isMjsFile) { | ||||||||||||||||
| return `import assert from "node:assert";${EOL}${sourceCode}`; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if (usingRequire || isCjsFile) { | ||||||||||||||||
| return `const assert = require("node:assert");${EOL}${sourceCode}`; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| const packageJsonPath = join(process.cwd(), 'package.json'); | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a more robust builtin way to find the package.json:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think since this check is last it should be okay: the migration has already determined that the file contents is ambiguous; this will forcibly disambiguate the file, but that's probably okay. I think best to add a note around here though that the sequence of checks needs to stay:
|
||||||||||||||||
| const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8')); | ||||||||||||||||
| const isEsm = packageJson.type === 'module'; | ||||||||||||||||
|
|
||||||||||||||||
| if (isEsm) { | ||||||||||||||||
| return `import assert from "node:assert";${EOL}${sourceCode}`; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return `const assert = require("node:assert");${EOL}${sourceCode}`; | ||||||||||||||||
| } | ||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import assert from "node:assert"; | ||
| assert(condition, "Basic assertion"); | ||
| assert.strictEqual(a, b, "Values should be equal"); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| const assert = require("node:assert"); | ||
| assert(condition, "Basic assertion"); | ||
| assert.strictEqual(a, b, "Values should be equal"); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import assert from "node:assert"; | ||
| assert(value, "Process assertion"); | ||
| assert.strictEqual(obj1, obj2); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import assert from "node:assert"; | ||
| import process from "node:process"; | ||
| assert(value, "Process assertion"); | ||
| process.env.NODE_ENV = "test"; | ||
| console.log(process.pid); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| const assert = require("node:assert"); | ||
| assert(value, "Process assertion"); | ||
| assert.throws(() => { throw new Error(); }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import assert from "node:assert"; | ||
| assert(condition, "Assertion from destructured import"); | ||
| assert.throws(() => { throw new Error("test"); }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| import assert from "node:assert"; | ||
| import { env } from "node:process"; | ||
| assert(value, "Using destructured assert"); | ||
| console.log(env.NODE_ENV); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import assert from "node:assert"; | ||
| assert(value, "Using aliased assert"); | ||
| assert.notStrictEqual(a, b); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import assert from "node:assert"; | ||
| import { env } from "node:process"; | ||
| assert(value, "Using aliased assert"); | ||
| assert.strictEqual(a, b); | ||
| console.log(env.NODE_ENV); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| const assert = require("node:assert"); | ||
| assert(value, "Destructured assert from require"); | ||
| assert.strictEqual(a, b, "Should be equal"); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import assert from "node:assert"; | ||
| assert(value, "This should be transformed"); | ||
| assert.strictEqual(a, b, "This should remain unchanged"); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| const assert = require("node:assert"); | ||
| assert(value, "This should be transformed"); | ||
| assert.strictEqual(a, b, "This should remain unchanged"); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| const config = { | ||
| port: 3000, | ||
| host: "localhost" | ||
| }; | ||
|
|
||
| console.log("Server config:", config); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import assert from "node:assert"; | ||
| function testFunction() { | ||
| assert(condition, "Assertion inside function"); | ||
|
|
||
| if (someCondition) { | ||
| assert.deepStrictEqual(obj1, obj2, "Deep comparison"); | ||
| } | ||
|
|
||
| return assert.ok(value) && true; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| import assert from "node:assert"; | ||
| assert(condition); | ||
| assert.ok(value); | ||
| assert.strictEqual(a, b); | ||
| assert.notStrictEqual(a, c); | ||
| assert.throws(() => { throw new Error(); }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| const assert = require("node:assert"); | ||
| const fs = require("fs"); | ||
|
|
||
| function readConfig(path) { | ||
| assert(fs.existsSync(path), "Config file must exist"); | ||
| const data = fs.readFileSync(path, "utf8"); | ||
| assert.ok(data.length > 0, "Config file cannot be empty"); | ||
| return JSON.parse(data); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has some changes that I think should not be here.