-
-
Couldn't load subscription status.
- Fork 20
feat(process-assert-to-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?
feat(process-assert-to-assert): introduce #200
Conversation
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.
I didn't see any test for that kind cases
import process from "node:process"But good first pr
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.
Awesome first contribution 🎉, Thank you @matheusmorett2 !
Just small fixes needed
| * Before: | ||
| * ```js | ||
| * process.assert(value); | ||
| * process.assert.strictEqual(a, b); | ||
| * ``` | ||
| * | ||
| * After: | ||
| * ```js | ||
| * import assert from "node:assert"; | ||
| * assert(value); | ||
| * assert.strictEqual(a, b); | ||
| * ``` |
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.
Remove process.assert.strictEqual reference, I think it was added by mistake. This scenario does not exist. Add bold formatting to the before/after text.
| * Before: | |
| * ```js | |
| * process.assert(value); | |
| * process.assert.strictEqual(a, b); | |
| * ``` | |
| * | |
| * After: | |
| * ```js | |
| * import assert from "node:assert"; | |
| * assert(value); | |
| * assert.strictEqual(a, b); | |
| * ``` | |
| * **Before**: | |
| * ```js | |
| * process.assert(value); | |
| * ``` | |
| * | |
| * **After**: | |
| * ```js | |
| * import assert from "node:assert"; | |
| * assert(value); | |
| * ``` |
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.
actually my code wasn't working for assert.strictEqual but is should
https://nodejs.org/api/assert.html
so I kept the assert.strictEqual
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.
Also added the bold on before and after words
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.
My point is that the TypeScript docs say that the code in the before block, process.assert.strictEqual(a, b), will be updated to use assert.strictEqual.
However, process.assert.strictEqual does not exist, at least I couldn’t find any reference to it.
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.
Please fix the conflict in the remove-binding file
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.
For require call not in cjs context the transformed code may be wrong.
For example this before
import { createRequire} from 'node:module';
const require = createRequire(import.meta.dirname)
process.assert(true);Broken after with the current implementation
const assert = require('node:assert');
import { createRequire} from 'node:module';
const require = createRequire(import.meta.dirname)
assert(true);In this case I expect that require is undefined.
Solutions:
- Try to found a logic that handle that.
- Add it at inline comment so we know for the future
|
|
||
| for (const processImport of allImports) { | ||
| const binding = resolveBindingPath(processImport, "$.assert"); | ||
| replaceRules.push({ |
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.
| replaceRules.push({ | |
| replaceRules.push({ |
|
|
||
| const processImportsToRemove = new Set<SgNode>(); | ||
|
|
||
| function processImports(moduleName: "process" | "node:process") { |
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.
| function processImports(moduleName: "process" | "node:process") { | |
| function processImports(moduleName: string) { |
Utilities used inside this function have regex to catch both usage of node:process or process. So you can simplify the logic of this function.
| if (binding) { | ||
| replaceRules.push({ | ||
| importNode: processImport, | ||
| binding, | ||
| rule: { | ||
| kind: "member_expression", | ||
| has: { | ||
| kind: "identifier", | ||
| regex: `^${binding}$`, | ||
| field: "object" | ||
| } | ||
| }, | ||
| replaceWith: "assert" | ||
| }); | ||
| } | ||
|
|
||
| const processUsages = rootNode.findAll({ | ||
| rule: { | ||
| kind: 'member_expression', | ||
| has: { | ||
| kind: 'identifier', | ||
| regex: '^process$' | ||
| } | ||
| } | ||
| }); |
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.
@brunocroh correct me if I am wrong but here dinging also include process so code after the condition is useless ?
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.
I think so
| { | ||
| kind: "pair_pattern", | ||
| }, | ||
| { | ||
| kind: "shorthand_property_identifier_pattern", | ||
| }, |
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.
| { | |
| kind: "pair_pattern", | |
| }, | |
| { | |
| kind: "shorthand_property_identifier_pattern", | |
| }, | |
| { | |
| kind: "pair_pattern" }, | |
| { kind: "shorthand_property_identifier_pattern" }, |
I found that simplest to read idk what you think about that
|
|
||
| This recipe transforms the usage of `process.assert` to use `assert` module. | ||
|
|
||
| See [DEP0100](https://github.com/nodejs/userland-migrations/issues/197). |
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.
| See [DEP0100](https://github.com/nodejs/userland-migrations/issues/197). | |
| See [DEP0100](https://nodejs.org/api/deprecations.html#DEP100). |
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.
Ps: verify the link I am on mobile for this review
|
hey @matheusmorett2 any news on this pr ? |
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.
I think this could use some refactoring. There's a lot of nesting, among other things.
Can you break it up, and look at the other mods for inspiration?
| const replaceRules: Array<{ | ||
| importNode?: SgNode; | ||
| binding?: string; | ||
| rule: Rule; | ||
| replaceWith?: string; | ||
| }> = [{ |
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.
Can we define this type outside as ReplaceRule?
| } | ||
|
|
||
| return sourceCode; | ||
| } No newline at end of file |
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.
| } | |
| } | |
| const isCommonJs = root.filename().includes('.cjs'); | ||
|
|
||
| if (Boolean(usingRequire) || isCommonJs) { | ||
| return `const assert = require("node:assert");${EOL}${sourceCode}`; |
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.
What if the assert variable is already defined?
| } | ||
| }); | ||
|
|
||
| const isCommonJs = root.filename().includes('.cjs'); |
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.
What if the filename is .cjs.mjs?
|
bump @matheusmorett2 |
Closes #197
Add codemod recipe
process-assert-to-assertto handle deprecation DEP0100.Before
After
Fix:
remove-binding.tshandle scenario where have alias in require