chore: replace get-stdin with node api, removed camelcase-keys #465
Conversation
🦋 Changeset detectedLatest commit: af05524 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes add a new contributor ("outslept") to both the contributors configuration and the README. The codebase removes the Changes
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to a0bd63a in 2 minutes and 24 seconds. Click for details.
- Reviewed
136lines of code in5files - Skipped
1files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .all-contributorsrc:288
- Draft comment:
New contributor added ('outslept'). Ensure that the contributor’s fields (login, avatar_url, etc.) stay consistent with the project guidelines. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure consistency with project guidelines, which is similar to asking them to double-check or verify something. It doesn't provide a specific suggestion or point out a specific issue with the code. Therefore, it violates the rules.
2. README.md:289
- Draft comment:
Contributor table row added for 'outslept' looks consistent. Double-check the avatar URL and links for correctness. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to double-check the avatar URL and links, which violates the rule against asking for confirmation or double-checking. It doesn't provide a specific suggestion or point out a clear issue.
3. package.json:52
- Draft comment:
Removed unused dependencies (camelcase-keys and get-stdin). This cleanup is good—verify that no indirect usage remains. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that no indirect usage remains, which violates the rule against asking for confirmation or verification. The first part of the comment is purely informative, which is also against the rules.
4. src/format-files.js:3
- Draft comment:
Replaced the get-stdin import with node’s stream consumer API. Usingtext(process.stdin)is cleaner and removes an extra dependency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining the change made in the code. It does not provide a suggestion, ask for confirmation, or point out a potential issue. According to the rules, purely informative comments should be removed.
5. src/format-files.spec.js:3
- Draft comment:
Updated tests to mock the newtextmethod from 'node:stream/consumers'. The changes correctly simulate stdin input; ensure that error cases (e.g. read failures) are also covered in tests if needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_bKnVQCaJJWWT9ziJ
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.all-contributorsrc (1)
291-299: Remember to bump the contributors-count badge in the README.A new contributor object is correctly added here, but the badge at the top of
README.mdstill showsall_contributors-21.
Please increment that value to22to keep the badge in sync.README.md (1)
290-292: Keep badge and list in sync after adding a new contributor.Great to see the new table row!
However, the badge declared earlier in the file still readsall_contributors-21, which will be misleading once this PR is merged.- [](#contributors) + [](#contributors)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.all-contributorsrc(1 hunks)README.md(1 hunks)package.json(0 hunks)src/format-files.js(2 hunks)src/format-files.spec.js(4 hunks)
💤 Files with no reviewable changes (1)
- package.json
🔇 Additional comments (4)
src/format-files.js (2)
3-3: Good modernization using native Node.js API.The import change from
get-stdinto the nativestream/consumersmodule is appropriate and eliminates an external dependency.
92-92: No Node.js compatibility issuesThe project’s package.json specifies
"node": "^18.18.0 || ^20.9.0 || >=21.1.0"and .nvmrc is set to 22.15.0, so usingstream/consumers(introduced in Node.js 16.7.0) poses no breaking change.src/format-files.spec.js (2)
3-3: Test mocking correctly updated for the new API.The mock setup properly replaces the
get-stdinmock with the nativetextfunction mock, maintaining test coverage.Also applies to: 17-21
75-76: Test cases properly updated for the new mock.The test implementations correctly use
mockText.mockResolvedValue()instead of the oldmockGetStdin.stdinapproach, maintaining the same test logic while adapting to the new API.Also applies to: 81-81, 94-94
|
Oopsie, there is a check missing. Fixing it |
The text() function uses or await (const chunk of stream) which will wait indefinitely for chunks when stdin is a TTY. get-stdin included TTY detection to return empty string immediately for terminal contexts. Added the same TTY check to maintain compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/format-files.js (1)
94-96: Fix redundant assignment.The assignment
stdinValue = '';on line 95 is redundant sincestdinValueis already initialized to an empty string on line 92.if (process.stdin.isTTY) { - stdinValue = ''; + // stdin is a TTY, no piped input available } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/format-files.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/format-files.js (1)
src/index.js (1)
logger(11-11)
🪛 ESLint
src/format-files.js
[error] 95-95: Review this redundant assignment: "stdinValue" already holds the assigned value along all execution paths.
(sonarjs/no-redundant-assignments)
🔇 Additional comments (3)
src/format-files.js (3)
3-3: Good modernization by using native Node.js API.Replacing the
get-stdinpackage with Node.js nativestream/consumersreduces external dependencies and is a good improvement.
97-108: Excellent error handling implementation.The try/catch block around
text(process.stdin)properly addresses the previous review comment about error handling. The implementation correctly:
- Handles potential errors during stdin reading
- Logs errors with appropriate context
- Sets the exit code
- Returns early with an empty string on error
This is a robust implementation that follows the existing error handling patterns in the codebase.
91-109: Confirmed Node.js CompatibilityThe project’s
engines.nodesetting is^18.18.0 || ^20.9.0 || >=21.1.0, which comfortably exceeds the v16.7.0 requirement for usingnode:stream/consumers. No changes needed.
|
CLI test return empty stdout/stderr locally on my machine. Process spawns successfully but produces no output. Likely environment-specific issue either only on my end or is platform specific. Primary OS is Windows 10 with WSL (Ubuntu / Arch) - getting failures on this specific test on all environments. Not sure how to debug further, will be happy if you can suggest ways to deal with this. |
commit: |
|
@outslept Thanks for your contribution first, while CI is broken. |
Resolved linter's warning |
|
I believe this is it, apologize a million times for any inconvenience during the review, but CLI test really stops me :( |
Signed-off-by: JounQin <[email protected]>
2 participants
replaced get-stdin with node api & removed camelcase-keys as it has 0 usage across the repo
about get-stdin: e18e/ecosystem-issues#159
p.s. cli tests hang on my machine and then fail after the timeout, I think the problem is in the environment on my part (although I don't know what exactly happened), so I pushed through --no-verify
Important
Replace
get-stdinwith Node.js API and remove unusedcamelcase-keyspackage.get-stdinwithnode:stream/consumersinsrc/format-files.jsandsrc/format-files.spec.js.camelcase-keysfrompackage.jsonas it is unused.formatStdin()insrc/format-files.jsto usetext(process.stdin)instead ofgetStdin().src/format-files.spec.jsto mocktextfromnode:stream/consumersinstead ofget-stdin..all-contributorsrcandREADME.md.This description was created by
for a0bd63a. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Chores
Refactor
Tests