feat: support cjs and esm both by tshy #6
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257
WalkthroughThis pull request introduces a comprehensive modernization of the Changes
Sequence DiagramsequenceDiagram
participant User
participant Pending
participant Callback
User->>Pending: Call with limit n
loop Until limit reached
User->>Pending: Invoke
Pending->>Pending: Count invocations
end
alt Invocations == n
Pending->>Callback: Execute
else Invocations > n
Pending->>User: Throw ExecuteTooManyTimesError
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
commit: |
|
Removed dependencies detected. Learn more about Socket for GitHub ↗︎ 🚮 Removed packages: npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/index.ts (2)
3-14: Consider a more descriptive name or additional metadata for the custom error class.While the class name conveys its purpose, you might want to add more detail to the message or provide additional payload in the error object for specialized error-handling in larger applications.
25-46: Evaluate partial-invocation scenarios carefully.The code stops invocation after the first reported error (i.e., sets “called = true”), which is consistent with many callback patterns. Ensure that this behavior aligns with your intended usage—especially if multiple possible errors could occur and only the first one is allowed to propagate. Also, if concurrency is expected, the increments to “times” and checks for “called” should be safe, but in Node.js single-threaded environments, this is typically acceptable.
README.md (2)
5-6: Helpful Node.js version badge additions.Using badges to showcase Node.js version requirements and welcoming PRs is a friendly approach to guide contributors.
25-38: Clear CommonJS usage example.Demonstrates typical usage with Node’s require syntax. Consider mentioning that “pedding” is deprecated, even if it is still supported, to gently steer users to “pending.”
.github/workflows/pkg.pr.new.yml (1)
12-16: Consider using Node.js LTS versionWhile Node.js 20 is fine, consider using the
lts/*version specifier for better maintenance.- run: corepack enable - uses: actions/setup-node@v4 with: - node-version: 20 + node-version: 'lts/*'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.eslintrc(1 hunks).github/workflows/nodejs.yml(1 hunks).github/workflows/pkg.pr.new.yml(1 hunks).github/workflows/release.yml(1 hunks).gitignore(1 hunks).jshintignore(0 hunks).jshintrc(0 hunks).npmignore(0 hunks).travis.yml(0 hunks)AUTHORS(0 hunks)LICENSE(1 hunks)README.md(1 hunks)component.json(0 hunks)index.js(0 hunks)package.json(1 hunks)src/index.ts(1 hunks)test/index.html(0 hunks)test/pedding.test.js(0 hunks)test/pedding.test.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (9)
- AUTHORS
- .npmignore
- .travis.yml
- component.json
- .jshintignore
- test/pedding.test.js
- index.js
- test/index.html
- .jshintrc
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- .eslintrc
- tsconfig.json
- LICENSE
🔇 Additional comments (21)
src/index.ts (3)
1-2: Good introduction of the Callback type.
Defining a clear function type improves maintainability and readability by communicating the expected parameter signature.
16-23: Maintain backward compatibility with caution.
Using type checks and reassigning parameters to handle the old usage pattern (pedding(fn, n)) is valid for backward compatibility. However, ensure the continued reliability of this approach for all legacy use cases and highlight the deprecation as you are doing with “pedding”.
48-51: Deprecation notice is properly placed.
Re-exporting “pedding” with a deprecation notice helps guide users toward “pending” while maintaining old functionality. This is a clear migration path.
test/pedding.test.ts (5)
1-12: Robust test for exceeding execution limit.
The test ensures that calling the callback more times than allowed triggers the correct error, increasing confidence in the “pending” function’s error-handling logic.
14-19: Clear demonstration of bounding multiple calls.
Using pedding(100, done) verifies that the callback can handle exactly the specified number of invocations without issues.
21-26: Good coverage of backward-compatibility usage.
Calling “pedding(done, 100)” ensures that the old usage pattern still works as originally intended.
28-40: Error-handling logic test is comprehensive.
Verifying that the callback is invoked only once after multiple errors ensures that the function’s error short-circuiting behaves properly.
42-55: Stack trace preservation is well-tested.
The check for “CallStack” in the error stack verifies that the original call location is captured, facilitating easier debugging of higher-level code.
README.md (4)
13-13: Typo correction improves clarity.
Changing “Just pedding for callback” to “Just pending for callback.” is consistent with the rename and helps avoid confusion.
20-20: Installation step is correct and concise.
This line properly documents the recommended installation approach.
41-51: ESM and TypeScript usage is detailed clearly.
The snippet effectively shows how to import and use “pending.” This meets modern JavaScript module standards.
59-65: License and Contributors sections are straightforward.
Linking to the MIT license and displaying contributor badges fosters a welcoming community approach.
package.json (9)
4-4: Description updated to reflect rename.
Changing from “Just pedding for callback.” to “Just pending for callback.” ensures clarity for first-time users, matching your new function naming.
8-10: Expanded keywords help discoverability.
Including both “pedding” and “pending” assists in bridging older references while ensuring that newer, more accurate search terms are present as well.
13-15: Node.js engine requirement is clear.
Dropping support for versions below 18.19.0 is a notable breaking change. Ensure you communicate this widely to users who may still rely on older Node versions.
17-26: Modern type definitions support.
Adopting TypeScript tooling and typed definitions for Node, Mocha, ESLint, etc., modernizes the project and is consistent with your new TypeScript code.
28-35: Well-structured scripts for linting and CI.
The synergy between “lint,” “test,” and “ci” clarifies your pipeline. The “prepublishOnly” step ensures compilation and other checks before publishing.
36-42: "type": "module" and "tshy" config for exports are well-configured.
Defining ES module usage plus a custom “tshy” config (likely used by a packaging or build tool) clarifies your module approach.
43-55: Granular exports remain robust.
Providing separate “import” and “require” fields ensures consumers can seamlessly adopt CommonJS or ESM. This is a best practice for maximum compatibility.
56-59: Including "dist" & "src" in “files” is consistent with TypeScript compilation.
This helps ensure that only the compiled output and associated source files are shipped, preventing clutter in your npm package.
60-62: "types", "main", and "module" are correctly set for TypeScript and Node.
Pointing TypeScript consumers to the correct d.ts file while providing separate ESM and CJS entry points covers all modern usage scenarios.
| uses: node-modules/github-actions/.github/workflows/node-release.yml@master | ||
| secrets: | ||
| NPM_TOKEN: ${{ secrets.NPM_TOKEN }} | ||
| GIT_TOKEN: ${{ secrets.GIT_TOKEN }} |
There was a problem hiding this comment.
Security: Pin external workflow version and add environment protection
- Using
@masterfor external workflows poses a security risk as the workflow could change unexpectedly. - Add environment protection rules for handling sensitive tokens.
- uses: node-modules/github-actions/.github/workflows/node-release.yml@master
+ uses: node-modules/github-actions/.github/workflows/node-release.yml@v1
+ environment: production
secrets:
NPM_TOKEN: ${{ secrets.NPM_TOKEN }}
GIT_TOKEN: ${{ secrets.GIT_TOKEN }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: node-modules/github-actions/.github/workflows/node-release.yml@master | |
| secrets: | |
| NPM_TOKEN: ${{ secrets.NPM_TOKEN }} | |
| GIT_TOKEN: ${{ secrets.GIT_TOKEN }} | |
| uses: node-modules/github-actions/.github/workflows/node-release.yml@v1 | |
| environment: production | |
| secrets: | |
| NPM_TOKEN: ${{ secrets.NPM_TOKEN }} | |
| GIT_TOKEN: ${{ secrets.GIT_TOKEN }} |
| on: | ||
| push: | ||
| branches: [ master ] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding version tag triggers
The workflow currently triggers on all pushes to master. Consider adding specific version tag triggers to ensure releases only happen for versioned changes.
on:
push:
branches: [ master ]
+ tags:
+ - 'v*'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| on: | |
| push: | |
| branches: [ master ] | |
| on: | |
| push: | |
| branches: [ master ] | |
| tags: | |
| - 'v*' | |
.github/workflows/nodejs.yml
Outdated
| uses: node-modules/github-actions/.github/workflows/node-test.yml@master | ||
| with: | ||
| os: 'ubuntu-latest' | ||
| version: '18.19.0, 18, 20, 22, 23' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security and reliability improvements needed
- Pin the external workflow version instead of using
@master - Consider adding fail-fast strategy configuration
- Node.js versions align well with the PR objective of dropping support for versions < 18.19.0
- uses: node-modules/github-actions/.github/workflows/node-test.yml@master
+ uses: node-modules/github-actions/.github/workflows/node-test.yml@v1
with:
os: 'ubuntu-latest'
version: '18.19.0, 18, 20, 22, 23'
+ fail-fast: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: node-modules/github-actions/.github/workflows/node-test.yml@master | |
| with: | |
| os: 'ubuntu-latest' | |
| version: '18.19.0, 18, 20, 22, 23' | |
| uses: node-modules/github-actions/.github/workflows/node-test.yml@v1 | |
| with: | |
| os: 'ubuntu-latest' | |
| version: '18.19.0, 18, 20, 22, 23' | |
| fail-fast: false |
.github/workflows/pkg.pr.new.yml
Outdated
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest | ||
|
|
There was a problem hiding this comment.
Add conditions to prevent publishing from forks
The job needs conditions to ensure it only runs in the appropriate context.
jobs:
build:
runs-on: ubuntu-latest
+ if: github.repository == 'node-modules/pedding' && github.event_name == 'push'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jobs: | |
| build: | |
| runs-on: ubuntu-latest | |
| jobs: | |
| build: | |
| runs-on: ubuntu-latest | |
| if: github.repository == 'node-modules/pedding' && github.event_name == 'push' | |
.github/workflows/pkg.pr.new.yml
Outdated
| - name: Install dependencies | ||
| run: npm install | ||
|
|
||
| - name: Build | ||
| run: npm run prepublishOnly --if-present | ||
|
|
||
| - run: npx pkg-pr-new publish |
There was a problem hiding this comment.
Add authentication and verification steps
The publishing steps are missing crucial authentication and verification steps.
- name: Install dependencies
run: npm install
- name: Build
run: npm run prepublishOnly --if-present
+ - name: Configure npm authentication
+ run: |
+ echo "//registry.npmjs.org/:_authToken=${{ secrets.NPM_TOKEN }}" > .npmrc
+ npm whoami
+ env:
+ NPM_TOKEN: ${{ secrets.NPM_TOKEN }}
+
+ - name: Verify package contents
+ run: npm pack --dry-run
+
- run: npx pkg-pr-new publish
+ env:
+ NPM_TOKEN: ${{ secrets.NPM_TOKEN }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Install dependencies | |
| run: npm install | |
| - name: Build | |
| run: npm run prepublishOnly --if-present | |
| - run: npx pkg-pr-new publish | |
| - name: Install dependencies | |
| run: npm install | |
| - name: Build | |
| run: npm run prepublishOnly --if-present | |
| - name: Configure npm authentication | |
| run: | | |
| echo "//registry.npmjs.org/:_authToken=${{ secrets.NPM_TOKEN }}" > .npmrc | |
| npm whoami | |
| env: | |
| NPM_TOKEN: ${{ secrets.NPM_TOKEN }} | |
| - name: Verify package contents | |
| run: npm pack --dry-run | |
| - run: npx pkg-pr-new publish | |
| env: | |
| NPM_TOKEN: ${{ secrets.NPM_TOKEN }} |
.github/workflows/pkg.pr.new.yml
Outdated
| name: Publish Any Commit | ||
| on: [push, pull_request] |
There was a problem hiding this comment.
Critical: Restrict publish workflow triggers
Publishing on every push and PR is dangerous. This could lead to unauthorized publishes from forks or unintended branches.
name: Publish Any Commit
-on: [push, pull_request]
+on:
+ push:
+ branches: [ master ]
+ tags:
+ - 'v*'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: Publish Any Commit | |
| on: [push, pull_request] | |
| name: Publish Any Commit | |
| on: | |
| push: | |
| branches: [ master ] | |
| tags: | |
| - 'v*' |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
[skip ci] ## [2.0.0](v1.1.0...v2.0.0) (2024-12-22) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced new workflows for continuous integration, package publishing, and release management. - Added TypeScript support with a new configuration file and strict type-checking. - New `pending` function implemented for managing callback execution limits. - **Bug Fixes** - Corrected function name from `pedding` to `pending` in usage examples. - **Documentation** - Updated `README.md` with new badges and improved usage examples. - Added MIT License to the project. - **Chores** - Removed outdated files and configurations, including `.jshintignore`, `.travis.yml`, `AUTHORS`, and `component.json`. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#6](#6)) ([d2352dc](d2352dc))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit
New Features
pendingfunction implemented for managing callback execution limits.Bug Fixes
peddingtopendingin usage examples.Documentation
README.mdwith new badges and improved usage examples.Chores
.jshintignore,.travis.yml,AUTHORS, andcomponent.json.