-
-
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
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.
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]>
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 code can be simplified using our utilities, and using more specific kinds at AST-grep rules.
I created this patch, please let me know what you think about it @Eliekhoury17
https://gist.github.com/brunocroh/7723a63af5b0f25f0f98d4475166f663
Okay I'm looking at it now to use your utilities and simplify the code. |
I just noticed that patch is applying a lot of code formatting changes. This happens because the previous code didn’t follow our Biome formatting rules. If you want me to create a new one without these style changes, let me know. (And sorry about that) By the way, the code will need to be formatted according to our Biome rules before we can merge this PR. |
Is it possible for you to explain a little bit what the link to the patch contains and what i need to change. I am new in theses things and I'm a little lost here on what to do in the code because i don't see what to do or use. @brunocroh |
I applied your patch on my code and all the test passes. So do I need to do something else than this or I can commit the changes? |
Oh, sorry, you just need to download the file on the link I share, and use |
Now you can review it on your side, check if you agree with the changes, make sure everything makes sense to you, and feel free to ask any questions about the differences. You can also fix any mistakes I might have made (for example, update comments, I didn’t touch them, so they might be outdated due to the logic changes). If the changes make sense, and you agree with them, you can create a new commit and push it for review again. Feel free to ask any questions you have, I’ll be glad to help! 😊 |
Thank you for the explanations :) . I checked everything, added the changes and modified one comment and committed the changes. For me everything looks good. You can review the commit . @brunocroh |
I checked everything, added the changes and committed the changes of your last patch and the tests and the code changes are good. For me everything looks good. @brunocroh |
Is there anything else you want me to do or modify in the code before it can be merged? |
I think all scenarios are covered. Great work, and thank you for your awesome contribution! 😁 |
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
.