-
-
Notifications
You must be signed in to change notification settings - Fork 19
feat(bytesRead-to-bytesWritten): implement Node.js DEP0108 migration #224
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(bytesRead-to-bytesWritten): implement Node.js DEP0108 migration #224
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.
WOW good for first issue 🎉, there are some thing to adjust but on logic/approach it's good for me.
The readme is cool IMO.
There are a missing part which is the dynamic import (also missing on the issue spec my bad 😅).
@brunocroh for that do the resolve binding already work with dynamic import ?
// In CommonJS
async function main() {
const zlib = await import("node:zlib");
const gzip = zlib.createGzip();
const processed = gzip.bytesWritten;
}
main();
// or in ESM
const zlib = await import("node:zlib");
const gzip = zlib.createGzip();
const processed = gzip.bytesWritten;
for the red ci you just have to resolve the new npm workspace on the monorepo for that just run npm I
👍
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.
Pull Request Overview
This PR implements a Node.js codemod to migrate from the deprecated zlib.bytesRead
property to zlib.bytesWritten
as part of DEP0108 deprecation handling. The codemod automatically transforms zlib stream usage across various import patterns and variable declarations.
- Implements comprehensive AST transformation logic to track zlib stream variables and replace deprecated property access
- Adds extensive test coverage for different import patterns (CommonJS, ESM, destructured, dynamic) and variable declarations
- Includes documentation and package configuration for the new codemod
Reviewed Changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/workflow.ts | Core transformation logic with import tracking, variable resolution, and property replacement |
workflow.yaml | Workflow configuration defining the AST transformation step |
tests/input/*.js | Test input files covering various zlib usage patterns |
tests/expected/*.js | Expected transformation outputs for test validation |
package.json | Package metadata and dependencies for the codemod |
codemod.yaml | Codemod registry configuration and metadata |
README.md | Documentation with examples and usage instructions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Augustin Mauroy <[email protected]>
Co-authored-by: Augustin Mauroy <[email protected]>
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.
Wow impressive. That nice some small review
Co-authored-by: Augustin Mauroy <[email protected]>
Co-authored-by: Augustin Mauroy <[email protected]>
Co-authored-by: Augustin Mauroy <[email protected]>
Co-authored-by: Augustin Mauroy <[email protected]>
Co-authored-by: Augustin Mauroy <[email protected]>
Co-authored-by: Augustin Mauroy <[email protected]>
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.
WOW impressive first contribution. For me everything is covered.
I hope you enjoyed this contribution.
} | ||
|
||
// 1.b Handle dynamic imports: `await import("node:zlib")` | ||
const dynamicImports = rootNode.findAll({ rule: { pattern: "const $$$VAR = await import($$$MODULE)" } }); |
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.
You just need to use $$$
(Triple dollar sign) when more than on values will be found, in both case, just one will be existing so it needs to be written with $
const dynamicImports = rootNode.findAll({ rule: { pattern: "const $VAR = await import($MODULE)" } });
btw, I think we have an utility that can resolve dynamic imports already, and this usage is not covering all cases, by example, if this line starts with var
or let
this not will match already.
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 did not find the utility that can resolve dynamic imports but I have covered the rest of the cases for let
and var
and replaced the $$$
with $
.
I committed the changes.
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 did not resolve convesation
. If you know if the utility that can resolve dynamic imports exists and how can I import it, I can use it and modify my code. If not, what i committed work very well and we can keep this version of the code and in this case, I will or if you can resolve 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.
LGTM 🙌
Before: | ||
|
||
```js | ||
const zlib = require("node:zlib"); | ||
const gzip = zlib.createGzip(); | ||
gzip.on("end", () => { | ||
console.log("Bytes processed:", gzip.bytesRead); | ||
}); | ||
``` | ||
|
||
After: | ||
|
||
```js | ||
const zlib = require("node:zlib"); | ||
const gzip = zlib.createGzip(); | ||
gzip.on("end", () => { | ||
console.log("Bytes processed:", gzip.bytesWritten); | ||
}); | ||
``` |
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.
@alexbit-codemod what was the verdict about diff
fenced blocks?
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.
@JakobJingleheimer FYI I asked for that on codemod slack
Co-authored-by: Jacob Smith <[email protected]>
This pull request implements the codemod to replace deprecated
zlib.bytesRead
with
zlib.bytesWritten
in Node.js transform streams, addressing DEP0108 (#196).Changes included:
.bytesRead
are replaced correctly.This PR is part of the feature branch
feat/bytesread-to-bytesWritten
.